From ae01a90187477eb9bb632c83c8ea848ce3d7a3f4 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 1 Nov 2026 22:50:38 +1100 Subject: [PATCH] Keep the listener table tests to ports they hold 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. --- src/DiffEngine.Tests/HeldPort.cs | 85 +++++++++++++++++++ src/DiffEngine.Tests/ListenerTableTests.cs | 80 ++++++++++------- src/DiffEngine.Tests/TrayDetectorTests.cs | 35 +++----- .../ViewerClientUnownedTests.cs | 38 +++++---- src/DiffEngine.Tests/ViewerProtocolTests.cs | 9 +- src/DiffEngine/Protocol/ListenerTable.cs | 46 ++++++++-- .../TrackerSnapshotTest.cs | 47 ++++------ 7 files changed, 222 insertions(+), 118 deletions(-) create mode 100644 src/DiffEngine.Tests/HeldPort.cs diff --git a/src/DiffEngine.Tests/HeldPort.cs b/src/DiffEngine.Tests/HeldPort.cs new file mode 100644 index 00000000..088f4772 --- /dev/null +++ b/src/DiffEngine.Tests/HeldPort.cs @@ -0,0 +1,85 @@ +/// +/// A port bound and not listening, for a test that needs one nobody is on. Bound rather than found +/// free and let go, so that nothing else is handed it while the test is using it: a test beside +/// this one binding port zero could otherwise be given the same port, and be told whatever this +/// test had the listener table say about it (), or be found +/// listening there by a test asking whether anybody is. +/// +sealed class HeldPort : IDisposable +{ + readonly Socket socket; + + public HeldPort() : + this(IPAddress.Loopback, 0) + { + } + + HeldPort(IPAddress address, int port) + { + socket = new(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp); + try + { + socket.Bind(new IPEndPoint(address, port)); + } + catch + { + socket.Dispose(); + throw; + } + + Port = ((IPEndPoint) socket.LocalEndPoint!).Port; + } + + /// + /// A listener of the test's own on , asked about by + /// while it listens and by once it has + /// stopped, with the port held again in between. The tests beside one bind port zero, and + /// Windows hands a port straight back out: asked about while free, a port was found listened + /// on by one of them within milliseconds. Where one had it first, this starts again on another. + /// + public static async Task AfterListening(IPAddress address, Func listening, Func stopped) + { + for (var attempt = 0; attempt < 10; attempt++) + { + var listener = new TcpListener(address, 0); + listener.Start(); + var port = ((IPEndPoint) listener.LocalEndpoint).Port; + try + { + await listening(port); + } + finally + { + listener.Stop(); + } + + using var held = TryAgain(address, port); + if (held is null) + { + continue; + } + + await stopped(port); + return; + } + + throw new("Every port was handed to something else before it could be held again."); + } + + static HeldPort? TryAgain(IPAddress address, int port) + { + try + { + return new(address, port); + } + catch (SocketException) + { + return null; + } + } + + public int Port { get; } + + public void Dispose() => + socket.Dispose(); +} diff --git a/src/DiffEngine.Tests/ListenerTableTests.cs b/src/DiffEngine.Tests/ListenerTableTests.cs index 3f357a66..0bfb531a 100644 --- a/src/DiffEngine.Tests/ListenerTableTests.cs +++ b/src/DiffEngine.Tests/ListenerTableTests.cs @@ -15,26 +15,53 @@ public class ListenerTableTests /// running is asked of it there. /// [Test] - public async Task AListenerIsListedOnEveryPlatform() + public Task AListenerIsListedOnEveryPlatform() => + HeldPort.AfterListening( + IPAddress.Loopback, + async _ => + { + await Assert.That(ListenerTable.IsListed(_)).IsTrue(); + await Assert.That(ListenerTable.Enumerated(_)).IsTrue(); + }, + async _ => await Assert.That(ListenerTable.IsListed(_)).IsFalse()); + + /// + /// Answers that overlap and end in either order leave the table answering for both ports, + /// which is what a lookup each test wrapped around the one before did not: the second test to + /// finish put back the first one's. + /// + [Test] + public async Task AnswersThatOverlapLeaveNothingBehind() { - var listener = new TcpListener(IPAddress.Loopback, 0); - listener.Start(); - var port = ((IPEndPoint) listener.LocalEndpoint).Port; - bool listed; - bool enumerated; - try - { - listed = ListenerTable.IsListed(port); - enumerated = ListenerTable.Enumerated(port); - } - finally - { - listener.Stop(); - } + using var first = new HeldPort(); + using var second = new HeldPort(); - await Assert.That(listed).IsTrue(); - await Assert.That(enumerated).IsTrue(); - await Assert.That(ListenerTable.IsListed(port)).IsFalse(); + var firstAnswer = ListenerTable.Answer(first.Port, () => true); + var secondAnswer = ListenerTable.Answer(second.Port, () => true); + await Assert.That(ListenerTable.IsListed(first.Port)).IsTrue(); + await Assert.That(ListenerTable.IsListed(second.Port)).IsTrue(); + + firstAnswer.Dispose(); + await Assert.That(ListenerTable.IsListed(first.Port)).IsFalse(); + await Assert.That(ListenerTable.IsListed(second.Port)).IsTrue(); + + secondAnswer.Dispose(); + await Assert.That(ListenerTable.IsListed(second.Port)).IsFalse(); + } + + /// + /// One answer a port, so two tests that believe they hold the same port find out, rather than + /// one quietly answering for the other. + /// + [Test] + public async Task APortIsAnsweredForOnce() + { + using var held = new HeldPort(); + using var answer = ListenerTable.Answer(held.Port, () => true); + + await Assert.That(() => ListenerTable.Answer(held.Port, () => false)) + .Throws(); + await Assert.That(ListenerTable.IsListed(held.Port)).IsTrue(); } [Test] @@ -96,19 +123,10 @@ public async Task AListenerSaysWhoseItIs() IPAddress[] addresses = Socket.OSSupportsIPv6 ? [IPAddress.Loopback, IPAddress.IPv6Loopback] : [IPAddress.Loopback]; foreach (var address in addresses) { - var listener = new TcpListener(address, 0); - listener.Start(); - var port = ((IPEndPoint) listener.LocalEndpoint).Port; - try - { - await Assert.That(ListenerTable.Owner(port)).IsEqualTo(current.Id); - } - finally - { - listener.Stop(); - } - - await Assert.That(ListenerTable.Owner(port)).IsNull(); + await HeldPort.AfterListening( + address, + async _ => await Assert.That(ListenerTable.Owner(_)).IsEqualTo(current.Id), + async _ => await Assert.That(ListenerTable.Owner(_)).IsNull()); } } diff --git a/src/DiffEngine.Tests/TrayDetectorTests.cs b/src/DiffEngine.Tests/TrayDetectorTests.cs index 24a2b17a..451825d3 100644 --- a/src/DiffEngine.Tests/TrayDetectorTests.cs +++ b/src/DiffEngine.Tests/TrayDetectorTests.cs @@ -7,24 +7,11 @@ public class TrayDetectorTests { [Test] [RunOn(TUnit.Core.Enums.OS.Linux | TUnit.Core.Enums.OS.MacOs)] - public async Task AListenerOnThePiperPortIsATray() - { - var listener = new TcpListener(IPAddress.Loopback, 0); - listener.Start(); - var port = ((IPEndPoint) listener.LocalEndpoint).Port; - bool running; - try - { - running = TrayDetector.IsRunning(port); - } - finally - { - listener.Stop(); - } - - await Assert.That(running).IsTrue(); - await Assert.That(TrayDetector.IsRunning(port)).IsFalse(); - } + public Task AListenerOnThePiperPortIsATray() => + HeldPort.AfterListening( + IPAddress.Loopback, + async _ => await Assert.That(TrayDetector.IsRunning(_)).IsTrue(), + async _ => await Assert.That(TrayDetector.IsRunning(_)).IsFalse()); /// /// No tray, where the piper's send would have taken it as one and let the connect decide: @@ -38,20 +25,18 @@ public async Task ATableThatCannotBeReadIsNoTray() var listener = new TcpListener(IPAddress.Loopback, 0); listener.Start(); var port = ((IPEndPoint) listener.LocalEndpoint).Port; - var previous = ListenerTable.Lookup; - // Every other port goes through untouched, since the tests beside this one are asking - // about theirs at the same time - ListenerTable.Lookup = _ => _ == port ? throw new("The table could not be read.") : previous(_); bool running; bool held; try { - running = TrayDetector.IsRunning(port); - held = ListenerTable.IsHeld(port); + using (ListenerTable.Answer(port, () => throw new("The table could not be read."))) + { + running = TrayDetector.IsRunning(port); + held = ListenerTable.IsHeld(port); + } } finally { - ListenerTable.Lookup = previous; listener.Stop(); } diff --git a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs index 537d23d4..a99bf5ea 100644 --- a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs +++ b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs @@ -168,13 +168,16 @@ public async Task AnOwnerStartedLaterIsFoundByTheTellingSends() public async Task APortAConnectFoundEmptyIsNotAskedAboutAgainSoSoon() { ViewerClient.RecheckUnlistedAfter = TimeSpan.Zero; - var port = FreePort(); + var held = new HeldPort(); + var port = held.Port; using var unreadable = new Lookup(port) { Unreadable = true }; await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); + // Let go only to be listened on, so the port is held again a moment later + held.Dispose(); using var owner = new Owner(port); await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); await Assert.That(owner.Heard).IsEmpty(); @@ -303,7 +306,8 @@ public async Task APortNobodyHoldsIsNotProbed() public async Task ATableThatCannotBeReadLeavesTheConnectToAnswer() { using var owner = new Owner(); - var dead = FreePort(); + using var held = new HeldPort(); + var dead = held.Port; using var unreadable = new Lookup(owner.Port, dead) { Unreadable = true @@ -457,19 +461,17 @@ await Assert.That(await ViewerClient.SendAsync(settle, Cancel.None, 0, skipIfUno /// Stands in front of the listener table for the ports a test names, counting how often each /// was asked about and, when told to, failing the way a table that cannot be read does, or /// saying nobody listens there whoever does. Every other port goes through untouched, since - /// the tests beside this one are asking about theirs at the same time. + /// the tests beside this one are asking about theirs at the same time. Each port named is one + /// the test holds ( or an ), or port zero, which + /// nothing is ever handed. /// sealed class Lookup : IDisposable { - readonly Func previous = ListenerTable.Lookup; - readonly int[] ports; + readonly IDisposable[] answers; int asked; - public Lookup(params int[] ports) - { - this.ports = ports; - ListenerTable.Lookup = Answer; - } + public Lookup(params int[] ports) => + answers = ports.Select(_ => ListenerTable.Answer(_, () => Answer(_))).ToArray(); public bool Unreadable { get; init; } @@ -479,11 +481,6 @@ public Lookup(params int[] ports) bool Answer(int port) { - if (!ports.Contains(port)) - { - return previous(port); - } - Interlocked.Increment(ref asked); if (Unreadable) { @@ -495,11 +492,16 @@ bool Answer(int port) return false; } - return previous(port); + return ListenerTable.Listed(port); } - public void Dispose() => - ListenerTable.Lookup = previous; + public void Dispose() + { + foreach (var answer in answers) + { + answer.Dispose(); + } + } } /// diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 3167c67f..6f6b27b9 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -1455,9 +1455,8 @@ await Assert.That(await ViewerClient.SendAsync(new(ViewerVerb.List), default, po [NotInParallel] public async Task AConnectGivenUpOnIsObserved() { - ViewerServer.TryBind(0, out var server); - var port = server!.Port; - server.Dispose(); + using var held = new HeldPort(); + var port = held.Port; ViewerClient.ForgetUnowned(); var unobserved = new List(); @@ -1469,8 +1468,7 @@ void Record(object? sender, UnobservedTaskExceptionEventArgs args) } } - var lookup = ListenerTable.Lookup; - ListenerTable.Lookup = _ => _ == port || lookup(_); + using var listed = ListenerTable.Answer(port, () => true); TaskScheduler.UnobservedTaskException += Record; try { @@ -1491,7 +1489,6 @@ void Record(object? sender, UnobservedTaskExceptionEventArgs args) finally { TaskScheduler.UnobservedTaskException -= Record; - ListenerTable.Lookup = lookup; ViewerClient.ForgetUnowned(); } diff --git a/src/DiffEngine/Protocol/ListenerTable.cs b/src/DiffEngine/Protocol/ListenerTable.cs index dc722323..bb7728c7 100644 --- a/src/DiffEngine/Protocol/ListenerTable.cs +++ b/src/DiffEngine/Protocol/ListenerTable.cs @@ -65,14 +65,48 @@ public static bool IsListed(int port) } /// - /// What and ask. Replaced by the tests, for the two things about the table - /// that cannot be arranged or seen from outside: that it could not be read, and that it was - /// not asked at all. Given the port, so a test can answer for its own and pass every other - /// through: the tests beside it are asking about theirs at the same time. + /// What and ask: a test's answer for the port, + /// where one stands (), and otherwise the table. /// - internal static Func Lookup { get; set; } = Listed; + static bool Lookup(int port) => + answers.TryGetValue(port, out var answer) ? answer() : Listed(port); - static bool Listed(int port) + static readonly ConcurrentDictionary> answers = new(); + + /// + /// Answers for in place of the table until disposed. For the tests, and + /// the two things about the table that cannot be arranged or seen from outside: that it could + /// not be read, which is an answer that throws, and that it was not asked at all, which is one + /// that counts. is the table's own answer, for one that passes through. + /// + /// One answer a port, kept apart from every other port's, since the tests beside one are asking + /// about theirs at the same time. It was a lookup a test replaced with one that wrapped the + /// lookup before it, and two tests that overlapped put each other's back: the one left + /// answered for its port for the rest of the run. + /// + /// + /// Only for a port the test holds, listening or bound, for as long as the answer stands. A free + /// port can be handed to a test that binds port zero, which is then told this answer about its + /// own listener. + /// + /// + internal static IDisposable Answer(int port, Func answer) + { + if (!answers.TryAdd(port, answer)) + { + throw new InvalidOperationException($"Port {port} is already answered for."); + } + + return new Answered(port, answer); + } + + sealed class Answered(int port, Func answer) : IDisposable + { + public void Dispose() => + ((ICollection>>) answers).Remove(new(port, answer)); + } + + internal static bool Listed(int port) { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Listeners(port) is { } held) diff --git a/src/DiffEngineTray.Core.Tests/TrackerSnapshotTest.cs b/src/DiffEngineTray.Core.Tests/TrackerSnapshotTest.cs index 1dd4959f..de79b808 100644 --- a/src/DiffEngineTray.Core.Tests/TrackerSnapshotTest.cs +++ b/src/DiffEngineTray.Core.Tests/TrackerSnapshotTest.cs @@ -225,29 +225,20 @@ public async Task TheOwnerIsLookedForInTheListenerTable() { using var viewer = new FakeViewer("Sample.cs:1"); var port = ViewerClient.Port; - var previous = ListenerTable.Lookup; var asked = 0; - ListenerTable.Lookup = _ => - { - if (_ == port) + using var counted = ListenerTable.Answer( + port, + () => { asked++; - } + return ListenerTable.Listed(port); + }); - return previous(_); - }; - try - { - var answered = new RemoteInlineHost().TryList(out var pending); + var answered = new RemoteInlineHost().TryList(out var pending); - await Assert.That(answered).IsTrue(); - await Assert.That(pending.Select(_ => _.Name)).IsEquivalentTo(["Sample.cs:1"]); - await Assert.That(asked).IsGreaterThan(0); - } - finally - { - ListenerTable.Lookup = previous; - } + await Assert.That(answered).IsTrue(); + await Assert.That(pending.Select(_ => _.Name)).IsEquivalentTo(["Sample.cs:1"]); + await Assert.That(asked).IsGreaterThan(0); } /// @@ -258,21 +249,13 @@ public async Task TheOwnerIsLookedForInTheListenerTable() public async Task NobodyInTheListenerTableIsNoOwner() { using var viewer = new FakeViewer("Sample.cs:1"); - var port = ViewerClient.Port; - var previous = ListenerTable.Lookup; - ListenerTable.Lookup = _ => _ != port && previous(_); - try - { - var answered = new RemoteInlineHost().TryList(out var pending); + using var unlisted = ListenerTable.Answer(ViewerClient.Port, () => false); - await Assert.That(answered).IsTrue(); - await Assert.That(pending).IsEmpty(); - await Assert.That(viewer.Verbs).IsEmpty(); - } - finally - { - ListenerTable.Lookup = previous; - } + var answered = new RemoteInlineHost().TryList(out var pending); + + await Assert.That(answered).IsTrue(); + await Assert.That(pending).IsEmpty(); + await Assert.That(viewer.Verbs).IsEmpty(); } ///