Repository navigation
Keep the listener table tests to ports they hold - #1016
Merged
Merged
Conversation
ListenerTableTests.AListenerIsListedOnEveryPlatform failed on CI's windows job (net48): having stopped its listener, it found the port still listed. Two things can make a port a test let go of say that. Windows hands a freed port straight back out, and the tests beside one bind port zero: locally AListenerSaysWhoseItIs found its stopped listener's port listened on by this same process within milliseconds. So the tests that ask what the table says once a listener stops hold the port again first, with a bound socket that does not listen (HeldPort.AfterListening), and start again on another port where something had it before them. And the tests told the table what to say through ListenerTable.Lookup, a delegate each replaced with one wrapping the one before and put back after. Two that overlapped put each other's back, and the one left answered for its port for the rest of the run. One of them, ViewerProtocolTests.AConnectGivenUpOnIsObserved, said a port it had let go of was listed, so whichever test was handed that port next was told so. Lookup is now a map of answers by port (ListenerTable.Answer), each removed by disposing what added it, and a second answer for a port that has one throws. A test answers only for a port it holds: the ones that let a free port go before answering for it hold a HeldPort now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ListenerTableTests.AListenerIsListedOnEveryPlatformfailed on thewindowsjob of VerifyTests/DiffEngine#1015, on net48, which touched nothing near it. After stopping its listener it still found the port listed. Two separate problems can make that happen.1. Windows hands out a port again as soon as it's freed. The tests beside this one bind port zero, so one of them can be given the port the moment it's let go. Locally,
AListenerSaysWhoseItIsfound its stopped listener's port in use by another listener in the same process within milliseconds.HeldPort(in the tests): a port bound but not listening, so nothing else can be given it.HeldPort.AfterListeningruns a test's question while its listener is up, stops the listener, and binds the port again before asking what the table says about it. If something got the port first, it starts again on a new port, up to ten times.ListenerTableTests.AListenerIsListedOnEveryPlatform;ListenerTableTests.AListenerSaysWhoseItIs;TrayDetectorTests.AListenerOnThePiperPortIsATray.2. The tests' replacements of
ListenerTable.Lookupcould leak.Lookupwas a delegate. Each test replaced it with one that wrapped the one before, and put the old one back afterwards. When two tests overlapped, the second to finish restored the first one's wrapper, which then answered for its port for the rest of the run.ViewerProtocolTests.AConnectGivenUpOnIsObservedsaid a port it had already freed was listed, so whichever test got that port next was told so.ListenerTable.Answer(port, answer)replaces the delegate. It keeps one answer per port in a map, and disposing what it returns removes that answer. A second answer for a port that already has one throws, so two tests that think they hold the same port find out.ListenerTable.Listedis now internal, for a test answer that passes through to the real table.The callers moved to it:
TrayDetectorTests;ViewerProtocolTests;Lookuphelper inViewerClientUnownedTests;TrackerSnapshotTest's two tests.Ports are held now: a test answers only for a port it holds. The three that freed a port before answering for it use a
HeldPortinstead:AConnectGivenUpOnIsObserved;ATableThatCannotBeReadLeavesTheConnectToAnswer;APortAConnectFoundEmptyIsNotAskedAboutAgainSoSoon.The last one lets its port go only to listen on it a moment later.
New tests in
ListenerTableTests:AnswersThatOverlapLeaveNothingBehindandAPortIsAnsweredForOnce.Tests. Locally on Windows, a Release build had no errors:
ListenerTable.Enumerated, which reads .NET's listener list. That list misses app listeners on this machine's Windows 10.0.26300 and fails the same way onmain.Lookupin place, a full run failedAListenerSaysWhoseItIson net48: "Expected to be null but found 11160", another listener in the same process on the freed port. That's what found the first problem. It didn't fail in either run afterHeldPort.AfterListening.Not seen working. I didn't reproduce the leaked replacement itself; the second problem is read from the code.
AListenerOnThePiperPortIsATrayruns only on Linux and macOS, so CI is the first run of its new form.