Change SelfHostDeployer to use dynamic ports by default #1383
Conversation
mikeharder
commented
Apr 16, 2018
mikeharder
commented
- Should significantly reduce flaky test failures due to AddressInUse exceptions
- Addresses SelfHostDeployer still produces AddressInUseException #1296
mikeharder
force-pushed
the
mikeharder/dynamic-port
branch
from
a4e2642 to
4a2eb61
Compare
| // once and never released. Binding to dynamic port "0" on "localhost" (both IPv4 and IPv6) is not | ||
| // supported, so the port is only bound on "127.0.0.1" (IPv4). If a test explicitly requires IPv6, | ||
| // it should provide a hint URL with "localhost" (IPv4 and IPv6) or "[::1]" (IPv6-only). | ||
| return new UriBuilder("http", "127.0.0.1", 0).Uri; |
There was a problem hiding this comment.
Why default to v4 rather than v6?
There was a problem hiding this comment.
If the test doesn't care, I assume IPv4 is safer since some environments don't support IPv6 (parts of Azure, maybe Travis/AppVeyor).
| { | ||
| if (string.IsNullOrEmpty(hint)) | ||
| { | ||
| return BuildTestUri(); | ||
| if (statusMessagesEnabled) |
There was a problem hiding this comment.
How is this going to work with HttpSys? It can't bind to port 0. Does it always have to set statusMessagesEnabled=false or provide a hint?
There was a problem hiding this comment.
Good catch, I assumed SelfHostDeployer was only used for Kestrel, but I now see it's also used for WebListener:
I see the following repos use ServerType.WebListener, I will run their tests with this change and see what happens:
- MusicStore
- ServerTests
- StaticFiles
- WebSockets
The best fix will likely be to pass DeploymentParameters.ServerType to BuildTestUri(), and use the less reliable GetNextPort() if ServerType == ServerType.WebListener.
| // If status messages are disabled, there is no status message from which to scrape the assigned port, | ||
| // so the less reliable GetNextPort() must be used. The port is bound on "localhost" (both IPv4 and IPv6), | ||
| // since this is supported when using a specific (non-zero) port. | ||
| return new UriBuilder("http", "localhost", GetNextPort()).Uri; |
There was a problem hiding this comment.
When we fall back to GetNextPort(), is it possible to add retry logic for address in use exceptions. If we can do that, it should eliminate all sources of flakiness.
There was a problem hiding this comment.
The problem is that logic lives elsewhere and involves more complicated scraping of the server process stdio. I think this is sufficient for a first pass
There was a problem hiding this comment.
Adding retries is possible but difficult. The AddressInUse exception doesn't come from GetNextPort(), it comes from the application started in an external process. So the retry logic would need to be here:
I think we'd need to try to start the app, and if it fails to start we'd need to scrape the output for AddressInUseException, and then retry on a different port. We'd need to be careful to not retry if it failed for any other reason. We'd also need to be careful to only retry if BuildTestUri() used GetNextPort(), since there's no point retrying if the test is using a hardcoded port.
Overall, this seems risky and error-prone, and it might cause more problems than it solves. I think we should wait and see how many flaky tests remain after changing the default to use a dynamic port.
There was a problem hiding this comment.
Yea it's just another enhancement we could add. Doesn't have to be in this PR which is why I approved it already.
from me, but you definitely want to also get someone super familiar with the nitty gritty parts of this to approve too.| } | ||
|
|
||
| public static Uri BuildTestUri(string hint) | ||
| { | ||
| return BuildTestUri(hint, statusMessagesEnabled: false); |
There was a problem hiding this comment.
I would have thought we would default to statusMessagesEnabled: true considering that you say below that it's the more reliable method?
There was a problem hiding this comment.
The TestUriHelper class and existing BuildTestUri() methods are public, so they can be called directly by test classes in other assemblies, not just by SelfHostDeployer. If the methods are called directly, we have no way of knowing if status messages are enabled or not, so we can't rely on them.
I made the new overload of BuildTestUri() internal to prevent other classes from calling it directly, since it should only be used by SelfHostDeployer. I will also review all existing usage of BuildTestUri(), BuildTestUri(string hint), and GetNextPort() and see if we can remove them or change them to internal.
| if (statusMessagesEnabled) | ||
| { | ||
| // Most functional tests use this codepath and should directly bind to dynamic port "0" and scrape | ||
| // the assigned port from the status message, which should be 100% reliable since the port is bound |
There was a problem hiding this comment.
Does SelfHostDeployer already scrape the assigned port from the status message? Does this mean that any test that uses SelfHostDeployer w/ Kestrel w/o specifying a port should just work?
There was a problem hiding this comment.
SelfHostDeployer already scrapes the assigned port when status messages are enabled (which is the default for most tests). So yes, once this change is merged and repos are updated to use this version, most tests should get more reliable with no other changes.
* Should significantly reduce flaky test failures due to AddressInUse exceptions * Addresses #1296
* Should significantly reduce flaky test failures due to AddressInUse exceptions * Addresses #1296
