Warn instead of throw when ignoring IServerAddressesFeature #1312
Conversation
halter73
commented
Jan 18, 2017
halter73
commented
- Throwing could be too much when IServerAddressesFeature URLs come from VS
| @@ -122,7 +122,7 @@ public void Start<TContext>(IHttpApplication<TContext> application) | |||
| if (addresses.SingleOrDefault() != "http://localhost:5000") | |||
| { | |||
| var joined = string.Join(", ", addresses); | |||
| throw new NotSupportedException($"Specifying address(es) '{joined}' is incompatible with also configuring endpoint(s) in UseKestrel."); | |||
| _logger.LogWarning($"Address(es) '{joined}' were overridden. Binding to endpoints defined in UseKestrel instead."); | |||
There was a problem hiding this comment.
nit: nameof(UseKestrel)
nit 2: add () after UseKestrel to make it clearer it's referring to the method call
There was a problem hiding this comment.
I think the wording needs to be a little different. It may be confusing to someone specifying multiple addresses in the feature but only overriding a single one of those in UseKestrel().
Maybe something like "Addresses in UseUrls()/IServerAddressesFeature [pick one] were [are? since it's something that will always happen when using both] overriden by the call to UseKestrel(). Binding to endpoints defined in UseKestrel() instead."
There was a problem hiding this comment.
Referencing UseUrls() or IServerAddressesFeature is the tricky part. UseUrls() might not be the thing that configured the addresses, and I doubt most ASP.NET Core devs know or care what IServerAddressesFeature is. That's why I just show the addresses that were configured.
| { | ||
| server.Features.Get<IServerAddressesFeature>().Addresses.Add("http://localhost:5001"); | ||
| StartDummyApplication(server); | ||
| Assert.Equal(1, testLogger.TotalWarningsLogged); |
There was a problem hiding this comment.
Check actual message. Also add more addresses to the feature (or better, make it a theory and have cases for single and multiple addresses).
| @@ -122,7 +122,7 @@ public void Start<TContext>(IHttpApplication<TContext> application) | |||
| if (addresses.SingleOrDefault() != "http://localhost:5000") | |||
There was a problem hiding this comment.
Since you don't throw anymore, remove this default check.
|
⬆️ 📅 |
| else | ||
| else if (!hasListenOptions && !hasServerAddresses) | ||
| { | ||
| listenOptions.Add(Constants.DefaultListenOptions); |
halter73
force-pushed
the
halter73/warn-addresses
branch
from
8830a8d to
d68471c
Compare
- Throwing could be too much when IServerAddressesFeature URLs come from VS - Listen on 127.0.0.1:5000 by default aspnet/Hosting#917
halter73
force-pushed
the
halter73/warn-addresses
branch
from
d68471c to
f32058c
Compare
