Add IServerAddressesFeature support#4685
Conversation
|
Multiple IIS test failures? |
|
Yep, I missed something. But in general PR ready for review. |
|
Failures are AuthSamples tests |
| _iisContextFactory = new IISContextFactory<TContext>(_memoryPool, application, _options, this, _logger); | ||
| _nativeApplication.RegisterCallbacks(_requestHandler, _shutdownHandler, _onDisconnect, _onAsyncCompletion, (IntPtr)_httpServerHandle, (IntPtr)_httpServerHandle); | ||
|
|
||
| Features.Set<IServerAddressesFeature>(new ServerAddressesFeature(_options.ServerAddresses)); |
There was a problem hiding this comment.
Clarification: The IServerAddressesFeature should be created and set in the IServer constructor, but the addresses should not be added until Start.
https://github.com/aspnet/KestrelHttpServer/blob/5c1fcd664d39db8fe5c8e38052a3cc29f90322f6/src/Kestrel.Core/KestrelServer.cs#L50-L51
Yes this is a bit of an arbitrary limitation, but we don't want people to get different behavior in IIS vs selfhost.
|
|
||
| for (auto binding : bindings) | ||
| { | ||
| result += binding.QueryProtocol() + L"://" + binding.QueryHost() + L":" + binding.QueryPort() + basePath + L";"; |
There was a problem hiding this comment.
Is pathBase encoded or decoded here? e.g. if I have a space in my base path do I get ' ' or %20? The escaped form should be preferred in this url form.
There was a problem hiding this comment.
The host is likely in unicode, no?
…lderIISExtensions.cs
| else if (selectedPort != bindingPort) | ||
| { | ||
| // If there are multiple endpoints configured return empty port | ||
| return L""; |
There was a problem hiding this comment.
environmentvariablehelpers is checking for null, not empty string. Or is there a difference here?
Fixes: #3843
This feature requires both new shim and new handler to work but is gracefully disabled otherwise.