diff --git a/claude.md b/claude.md index 931de197..e7659518 100644 --- a/claude.md +++ b/claude.md @@ -321,5 +321,6 @@ apart. - Tool discovery uses wildcard path matching (`WildcardFileFinder`) to find executables in common install locations - Tool order can be customized via `DiffEngine_ToolOrder` environment variable - `DisabledChecker` respects `DiffEngine_Disabled` env var +- `ViewerClient` remembers a port found unowned for ten minutes (`RecheckUnownedAfter`), and the library's telling sends - settle, retire, move, delete, the first inline or diff send - skip the connect while that stands. A refused loopback connection costs two seconds on Windows (firewall stealth mode drops the reset), and a green run settles once per inline verification, which was six minutes for a class of 188 inline tests. Probes (`IsOwned`), the hosts and `InlineQueueClient` always connect and correct the memory; so does `SettleAppliedInline`, being one send per accept - `TrayDisabledChecker` respects `DiffEngine_TrayDisabled` env var, behind `DiffRunner.TrayDisabled`. Separate from `Disabled` because tracking a pending move is separate from launching a tool: every exit of `InnerLaunch`, `Disabled` included, still calls `AddMove`. `PendingFiles.TrayAvailable` is the single gate - Tests use TUnit and Verify for snapshot testing diff --git a/docs/inline.md b/docs/inline.md index 4575c23b..948458d2 100644 --- a/docs/inline.md +++ b/docs/inline.md @@ -77,6 +77,7 @@ For the producing side — a test library with a failing inline snapshot: * `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. * `DiffRunner.SettleInline(sourceFile, line)` drops the pending entry for a call site, for when a previously failing test passes. Unknown entries and an absent owner are no-ops, so call it freely. The settle carries the running framework, so a multi-targeted run only settles its own variant of a conflicted entry. That framework is the running process's, which makes this the test run's verb and only the test run's: a surface applying a patch of its own wants `SettleAppliedInline`, [below](#applying-a-patch-from-another-surface). +* An absent owner is also remembered. A port found with nothing listening is taken as still unowned for ten minutes, and the sends that only tell the owner something — settle, retire, a move or delete to track, the first attempt to queue a patch — return without connecting while that stands. A refused loopback connection is not free on Windows: the firewall's stealth mode, on by default, drops the reset a closed port would answer with, so each refusal takes two seconds, and a green run settling once per inline verification was spending minutes on them. Anything that has to reach an owner probes for itself before launching a viewer, and that probe, like every listing, always connects and corrects the memory with what it finds. * `AddInlineAsync` stamps `patch.Framework` with the consuming project's target framework ("net9.0", "net48") unless the caller already set it, which is what lets the owner tell a re-run from another framework disagreeing. The value is the `$(TargetFramework)` the package's build targets stamp into the project's runtimeconfig, read back rather than asked of the process — in a hosted test run the entry assembly is the runner (testhost, ReSharperTestRunner), whose framework is not the project's — with the running runtime's version as the fallback for consumers without the targets. Callers may also set `patch.TestName`, which the viewer uses to group and label the queue; without it, items are labeled by call site. * Set `patch.OriginalExpression` from `CallerArgumentExpression` where the language supplies one, and `patch.OriginalValue` — the previous expected argument's value — where it does not. One of the two is what stops a patch rewriting the wrong call site when the file has moved since the run. `patch.MemberName` from `CallerMemberName` narrows it further, and is supported everywhere including F#. * Setting `DiffEngine_InlineViewer` to `false` reports `NoViewerFound` without probing, which is how a user opts into reviewing in their IDE instead of a window. @@ -199,7 +200,7 @@ A review surface that is neither the viewer nor the tray — the ReSharper / Rid `Accept` returns `Accepted` (the entry has gone), `Failed` (still pending and retryable — a locked file, or a conflicted entry that has to be resolved in the viewer), or `Unknown` (nobody owns the queue, or it holds nothing for that call site). It hands back the owner's own message, which is what tells an applied patch from a stale one that was dropped: the wire carries whether the verb was carried out rather than an apply status, so from a client those are the same observation, and only the words separate them. -Listing waits half a second, since an owner that cannot answer one in that time is wedged rather than slow, and a refused connection is immediate either way. Accepting waits fifteen seconds, because the owner applies through `InlineApplier` and that can sit on its cross process mutex for ten. +Listing waits half a second, since an owner that cannot answer one in that time is wedged rather than slow. A refused connection is immediate on most machines, though not on Windows, where the firewall's stealth mode holds the connect for two seconds; a listing asks every time regardless, because a surface lists precisely to notice an owner arriving, and it is only the test run's fire and forget sends that remember an absent owner for ten minutes instead. Accepting waits fifteen seconds, because the owner applies through `InlineApplier` and that can sit on its cross process mutex for ten. ## Applying a patch from another surface diff --git a/docs/mdsource/inline.source.md b/docs/mdsource/inline.source.md index f7bd40e3..254e2907 100644 --- a/docs/mdsource/inline.source.md +++ b/docs/mdsource/inline.source.md @@ -70,6 +70,7 @@ For the producing side — a test library with a failing inline snapshot: * `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. * `DiffRunner.SettleInline(sourceFile, line)` drops the pending entry for a call site, for when a previously failing test passes. Unknown entries and an absent owner are no-ops, so call it freely. The settle carries the running framework, so a multi-targeted run only settles its own variant of a conflicted entry. That framework is the running process's, which makes this the test run's verb and only the test run's: a surface applying a patch of its own wants `SettleAppliedInline`, [below](#applying-a-patch-from-another-surface). +* An absent owner is also remembered. A port found with nothing listening is taken as still unowned for ten minutes, and the sends that only tell the owner something — settle, retire, a move or delete to track, the first attempt to queue a patch — return without connecting while that stands. A refused loopback connection is not free on Windows: the firewall's stealth mode, on by default, drops the reset a closed port would answer with, so each refusal takes two seconds, and a green run settling once per inline verification was spending minutes on them. Anything that has to reach an owner probes for itself before launching a viewer, and that probe, like every listing, always connects and corrects the memory with what it finds. * `AddInlineAsync` stamps `patch.Framework` with the consuming project's target framework ("net9.0", "net48") unless the caller already set it, which is what lets the owner tell a re-run from another framework disagreeing. The value is the `$(TargetFramework)` the package's build targets stamp into the project's runtimeconfig, read back rather than asked of the process — in a hosted test run the entry assembly is the runner (testhost, ReSharperTestRunner), whose framework is not the project's — with the running runtime's version as the fallback for consumers without the targets. Callers may also set `patch.TestName`, which the viewer uses to group and label the queue; without it, items are labeled by call site. * Set `patch.OriginalExpression` from `CallerArgumentExpression` where the language supplies one, and `patch.OriginalValue` — the previous expected argument's value — where it does not. One of the two is what stops a patch rewriting the wrong call site when the file has moved since the run. `patch.MemberName` from `CallerMemberName` narrows it further, and is supported everywhere including F#. * Setting `DiffEngine_InlineViewer` to `false` reports `NoViewerFound` without probing, which is how a user opts into reviewing in their IDE instead of a window. @@ -192,7 +193,7 @@ A review surface that is neither the viewer nor the tray — the ReSharper / Rid `Accept` returns `Accepted` (the entry has gone), `Failed` (still pending and retryable — a locked file, or a conflicted entry that has to be resolved in the viewer), or `Unknown` (nobody owns the queue, or it holds nothing for that call site). It hands back the owner's own message, which is what tells an applied patch from a stale one that was dropped: the wire carries whether the verb was carried out rather than an apply status, so from a client those are the same observation, and only the words separate them. -Listing waits half a second, since an owner that cannot answer one in that time is wedged rather than slow, and a refused connection is immediate either way. Accepting waits fifteen seconds, because the owner applies through `InlineApplier` and that can sit on its cross process mutex for ten. +Listing waits half a second, since an owner that cannot answer one in that time is wedged rather than slow. A refused connection is immediate on most machines, though not on Windows, where the firewall's stealth mode holds the connect for two seconds; a listing asks every time regardless, because a surface lists precisely to notice an owner arriving, and it is only the test run's fire and forget sends that remember an absent owner for ten minutes instead. Accepting waits fifteen seconds, because the owner applies through `InlineApplier` and that can sit on its cross process mutex for ten. ## Applying a patch from another surface diff --git a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs new file mode 100644 index 00000000..e7f48cff --- /dev/null +++ b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs @@ -0,0 +1,231 @@ +/// +/// The memory of a port nothing was listening on, which is what stops a green run paying for a +/// refused connection once per inline verification. +/// +/// A refused loopback connection is instant on most machines and two seconds on Windows with the +/// firewall's stealth mode on, which is the default. Each test here meets one refusal at most, on +/// an ephemeral port of its own, so it costs that once and never talks to the live viewer port. +/// +/// +[NotInParallel] +public class ViewerClientUnownedTests +{ + static readonly ViewerMessage settle = new(ViewerVerb.Settle, InlineKey.For("Tests.cs", 1)); + + // Read before any test can have changed it, so the restore below puts back the real default + // rather than a copy of it kept here + static readonly TimeSpan recheckUnownedAfter = ViewerClient.RecheckUnownedAfter; + + [Before(Test)] + public void Forget() => + ViewerClient.ForgetUnowned(); + + [After(Test)] + public void Restore() + { + ViewerClient.RecheckUnownedAfter = recheckUnownedAfter; + ViewerClient.ForgetUnowned(); + } + + /// + /// The first refusal is remembered, so the next telling send returns without connecting - and + /// keeps returning that way once somebody is there, until something asks. + /// + [Test] + public async Task ARefusalIsRemembered() + { + var port = FreePort(); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); + + using var owner = new Owner(port); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); + await Assert.That(owner.Heard).IsEmpty(); + } + + /// + /// An ask always connects, and having found the owner, corrects the memory for the sends + /// after it. + /// + [Test] + public async Task AnAskCorrectsTheMemory() + { + var port = FreePort(); + ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true); + using var owner = new Owner(port); + + await Assert.That(ViewerClient.TrySend(settle, out _, port)).IsTrue(); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(2); + } + + /// + /// The probe is the ask that matters most. The launch gate runs it once a viewer is started, + /// and the sends queued behind the gate have to reach the viewer it found. + /// + [Test] + public async Task TheProbeCorrectsTheMemory() + { + var port = FreePort(); + ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true); + using var owner = new Owner(port); + + await Assert.That(ViewerClient.IsOwned(port)).IsTrue(); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(1); + } + + /// + /// A probe finding nobody is remembered too. The gate probes once per launch, and a viewer + /// that never binds would otherwise leave every send in the run paying for the refusal. + /// + [Test] + public async Task AProbeFindingNobodyIsRemembered() + { + var port = FreePort(); + await Assert.That(ViewerClient.IsOwned(port)).IsFalse(); + + using var owner = new Owner(port); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); + await Assert.That(owner.Heard).IsEmpty(); + } + + /// + /// A tray started mid run is found once the memory has expired, without anything asking. + /// + [Test] + public async Task TheMemoryExpires() + { + ViewerClient.RecheckUnownedAfter = TimeSpan.Zero; + var port = FreePort(); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsFalse(); + + using var owner = new Owner(port); + await Assert.That(ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(1); + } + + /// + /// Per port, or a test suite's dead ephemeral port would silence the sends to its live one. + /// + [Test] + public async Task PortsAreRememberedApart() + { + var dead = FreePort(); + using var owner = new Owner(); + await Assert.That(ViewerClient.TrySend(settle, out _, dead, skipIfUnowned: true)).IsFalse(); + + await Assert.That(ViewerClient.TrySend(settle, out _, owner.Port, skipIfUnowned: true)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(1); + } + + [Test] + public async Task TheAsyncSendRemembersToo() + { + var port = FreePort(); + await Assert.That(await ViewerClient.SendAsync(settle, Cancel.None, port, skipIfUnowned: true)) + .IsEqualTo(SendOutcome.NoOwner); + + using var owner = new Owner(port); + await Assert.That(await ViewerClient.SendAsync(settle, Cancel.None, port, skipIfUnowned: true)) + .IsEqualTo(SendOutcome.NoOwner); + await Assert.That(owner.Heard).IsEmpty(); + + await Assert.That(ViewerClient.IsOwned(port)).IsTrue(); + await Assert.That(await ViewerClient.SendAsync(settle, Cancel.None, port, skipIfUnowned: true)) + .IsEqualTo(SendOutcome.Accepted); + } + + /// + /// A caller that does not say is asking, and an ask is never answered from memory. This is + /// what keeps the hosts and the IDE plugin, which share this client, noticing an owner arrive. + /// + [Test] + public async Task AskingIsTheDefault() + { + var port = FreePort(); + ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true); + using var owner = new Owner(port); + + await Assert.That(await ViewerClient.SendAsync(settle, Cancel.None, port)).IsEqualTo(SendOutcome.Accepted); + await Assert.That(ViewerClient.TrySend(settle, out _, port)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(2); + } + + /// + /// A port that is free right now, found by binding and releasing it. + /// + static int FreePort() + { + if (!ViewerServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + var port = bound.Port; + bound.Dispose(); + return port; + } + + /// + /// A queue owner on a port of the test's choosing, answering every verb and recording it. + /// Zero binds an ephemeral port, for a test that only needs somebody to be there. + /// + sealed class Owner : IDisposable + { + readonly ViewerServer server; + readonly CancelSource cancel = new(); + readonly Task listening; + readonly Lock gate = new(); + readonly List heard = []; + + public Owner(int port = 0) + { + if (!ViewerServer.TryBind(port, out var bound)) + { + throw new($"Could not bind port {port}."); + } + + server = bound; + listening = server.Listen( + _ => + { + lock (gate) + { + heard.Add(_.Verb); + } + + return ViewerResponse.Success(); + }, + cancel.Token); + } + + public int Port => server.Port; + + public IReadOnlyList Heard + { + get + { + lock (gate) + { + return heard.ToList(); + } + } + } + + public void Dispose() + { + cancel.Cancel(); + server.Dispose(); + try + { + listening.Wait(TimeSpan.FromSeconds(2)); + } + catch (AggregateException) + { + // Cancellation unwinding through the listener; nothing to report + } + + cancel.Dispose(); + } + } +} diff --git a/src/DiffEngine/DiffRunner_Inline.cs b/src/DiffEngine/DiffRunner_Inline.cs index cce3a382..42a64f45 100644 --- a/src/DiffEngine/DiffRunner_Inline.cs +++ b/src/DiffEngine/DiffRunner_Inline.cs @@ -63,7 +63,10 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel // Onto the payload rather than onto the patch, which belongs to the caller and may be // held or sent again var payload = InlinePatchFile.Build(patch, patch.Framework ?? RuntimeMoniker.Current); - var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel); + // A port recently found unowned is not asked again here, because the gate below probes for + // itself before launching anything, and that probe is what corrects the memory when an + // owner has arrived since. Asking twice would only pay twice for the same refusal + var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel, skipIfUnowned: true); if (outcome == SendOutcome.Accepted) { return InlineResult.Queued; @@ -90,7 +93,11 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel /// /// Drops a pending inline snapshot from the viewer's queue, for when a previously failing test - /// starts passing. Does nothing when no viewer is running. + /// starts passing. Does nothing when no viewer is running - and cheaply, since this is called + /// once per passing inline verification: a port found with nothing listening is taken as + /// still unowned for ten minutes rather than connected to again, because on Windows a refused + /// loopback connection takes two seconds rather than none, and a green run of a few hundred + /// inline tests was spending minutes on refusals. /// /// Carries this process's framework so a multi-targeted run only settles its own variant of a /// conflicted entry; the other framework's differing content stays pending. @@ -165,8 +172,13 @@ public static void SettleAppliedInline(InlinePatch patch) return; } + // Not answered from the memory of an unowned port the way the test run's own settles are. + // This is one send per accept on a person's click rather than one per verification, and an + // owner that has appeared since an earlier refusal - a viewer a later test run started - + // holds exactly the entry this is for. So it always connects, whatever was found before ViewerClient.TrySend( - new(ViewerVerb.Settle, InlineKey.For(patch.SourceFile, patch.LineHint), null, patch.MemberName)); + new(ViewerVerb.Settle, InlineKey.For(patch.SourceFile, patch.LineHint), null, patch.MemberName), + out _); } static InlineResult CheckInline() diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index d54a870a..9e091597 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -3,7 +3,9 @@ namespace DiffEngine; /// /// Talks to whoever owns the inline queue. A refused connection means nobody does, which the /// caller turns into a launch (DiffEngine), "nothing pending" (the tray), or "the owner has gone" -/// (an attached viewer). +/// (an attached viewer). It is also remembered, per port and for a while, so the sends that are +/// only telling the owner something do not pay for the same refusal once per test: see +/// . /// /// /// What came back from an exchange with the queue owner. Three outcomes rather than two, because @@ -80,30 +82,110 @@ public static int Port /// public static readonly TimeSpan ShortTimeout = TimeSpan.FromMilliseconds(500); + /// + /// How long a port found with nothing listening stands as unowned, during which the sends + /// that only tell the owner something return without connecting. + /// + /// A refused loopback connection is not free everywhere. Windows Firewall's stealth mode, on + /// by default, drops the reset a closed port would answer with, so the connect sits through + /// the SYN retries and fails after two seconds rather than at once. A passing inline snapshot + /// settles once per verification, and nothing owning the queue is the ordinary state of a + /// machine with no tray - so a green run of two hundred inline tests spent six minutes + /// connecting to nobody, and the same run took under a second with a tray answering. + /// + /// + /// Long, because a recheck on such a machine costs those two seconds again and buys almost + /// nothing. What the memory can delay is only a message the owner did not have to receive: + /// an entry to settle in a queue that did not exist when the test failed, or a move to track + /// in a tray that was not there to track it. Anything that has to reach an owner - a patch, a + /// delete, a pair - goes through the launch gate, whose probe always + /// connects and corrects the memory with what it finds. Not the life of the process only for + /// a long lived consumer that is not a test host, launching diff tools all day, where a tray + /// started later would otherwise never see its moves until a restart. + /// + /// + internal static TimeSpan RecheckUnownedAfter { get; set; } = TimeSpan.FromMinutes(10); + + /// + /// When each port was last found unowned, as a timestamp. Per port + /// because tests talk to ephemeral ports of their own, in parallel, and what happened on + /// those says nothing about the one live port on a developer machine. + /// + static readonly ConcurrentDictionary unownedAt = new(); + + static bool RecentlyUnowned(int port) + { + if (!unownedAt.TryGetValue(port, out var at)) + { + return false; + } + + var elapsed = TimeSpan.FromSeconds((Stopwatch.GetTimestamp() - at) / (double) Stopwatch.Frequency); + return elapsed < RecheckUnownedAfter; + } + + /// + /// What a connect found, reported by every connect here whether or not its caller consulted + /// the memory first. An owner found by a probe or a listing is one every later send can talk + /// to, and a refusal met by any of them is what the memory is for. + /// + static void Found(int port, bool owned) + { + if (owned) + { + unownedAt.TryRemove(port, out _); + } + else + { + unownedAt[port] = Stopwatch.GetTimestamp(); + } + } + + /// + /// For tests, which share this process and its memory with every other test's ports. + /// + internal static void ForgetUnowned() => + unownedAt.Clear(); + /// /// Whether anything is listening, without sending it anything. For a caller that has just /// started a viewer and wants to know when it can be talked to, which a send cannot answer /// without also handing over work. + /// + /// Always connects, and what it finds corrects the memory behind + /// : this is the probe the launch gate runs once a viewer + /// is started, and the sends queued behind that gate have to reach the viewer it found. + /// /// public static bool IsOwned(int? port = null) { + var endpointPort = port ?? Port; + bool owned; try { using var client = new TcpClient(); - return client.ConnectAsync(IPAddress.Loopback, port ?? Port).Wait(ShortTimeout); + owned = client.ConnectAsync(IPAddress.Loopback, endpointPort).Wait(ShortTimeout); } catch (Exception exception) when (Ignorable(exception)) { - return false; + owned = false; } + + Found(endpointPort, owned); + return owned; } /// /// True when the owner acknowledged. A refused connection means nobody owns the queue. + /// + /// Every caller of this overload is telling the owner something rather than asking it - a + /// settle, a retire, a move or a delete to track - so a port recently found unowned is taken + /// at its word rather than connected to again: see . + /// /// public static bool TrySend(ViewerMessage message) => - TrySend(message, out var response) && + TrySend(message, out var response, skipIfUnowned: true) && response.Ok; /// @@ -113,23 +195,42 @@ public static bool TrySend(ViewerMessage message) => /// overrides for a single call. Tests pass their own /// ephemeral port rather than mutating anything static, so they can run in parallel. /// + /// + /// lets a port found unowned within + /// answer for itself, with no connect. Off by default, + /// because a host or a review surface asks precisely so that it notices an owner arriving, + /// and a listing answered from what was found ten minutes ago is a listing that misses one. + /// The library's own sends pass true. + /// /// public static bool TrySend( ViewerMessage message, [NotNullWhen(true)] out ViewerResponse? response, int? port = null, - TimeSpan? wait = null) + TimeSpan? wait = null, + bool skipIfUnowned = false) { response = null; + var endpointPort = port ?? Port; + if (skipIfUnowned && + RecentlyUnowned(endpointPort)) + { + return false; + } + var deadline = wait ?? timeout; + var connected = false; try { using var client = new TcpClient(); - if (!client.ConnectAsync(IPAddress.Loopback, port ?? Port).Wait(deadline)) + if (!client.ConnectAsync(IPAddress.Loopback, endpointPort).Wait(deadline)) { + Found(endpointPort, false); return false; } + connected = true; + Found(endpointPort, true); Configure(client, deadline); var stream = client.GetStream(); var bytes = Encoding.UTF8.GetBytes(message.Build()); @@ -142,6 +243,13 @@ public static bool TrySend( catch (Exception exception) when (Ignorable(exception)) { + // Only a connect that failed says the port is unowned. A connection that was accepted + // and then torn down is an owner behaving badly, which is not what the memory records + if (!connected) + { + Found(endpointPort, false); + } + return false; } } @@ -162,24 +270,39 @@ public static async Task TrySendAsync( Cancel cancel, int? port = null, TimeSpan? wait = null) => - await SendAsync(message, cancel, port, wait) == SendOutcome.Accepted; + // Telling rather than asking, as the synchronous bool overload is, and so answered from + // the memory of an unowned port the same way + await SendAsync(message, cancel, port, wait, skipIfUnowned: true) == SendOutcome.Accepted; /// /// As , but says which of the two failures happened. A caller that /// would launch a viewer on absence needs that: launching one because the owner refused the /// payload leaves two processes and still no snapshot. + /// + /// is as on the synchronous overload: a port found unowned + /// within reports with no + /// connect, which suits a caller about to probe for itself through the launch gate. + /// /// public static async Task SendAsync( ViewerMessage message, Cancel cancel, int? port = null, - TimeSpan? wait = null) + TimeSpan? wait = null, + bool skipIfUnowned = false) { var endpointPort = port ?? Port; + if (skipIfUnowned && + RecentlyUnowned(endpointPort)) + { + return SendOutcome.NoOwner; + } + var timeToWait = wait ?? asyncTimeout; using var deadline = CancelSource.CreateLinkedTokenSource(cancel); deadline.CancelAfter(timeToWait); var token = deadline.Token; + var connected = false; try { using var client = new TcpClient(); @@ -201,6 +324,8 @@ public static async Task SendAsync( // caller is written against token.ThrowIfCancellationRequested(); #endif + connected = true; + Found(endpointPort, true); Configure(client, timeToWait); var stream = client.GetStream(); var bytes = Encoding.UTF8.GetBytes(message.Build()); @@ -232,16 +357,30 @@ public static async Task SendAsync( catch (Exception exception) when (!cancel.IsCancellationRequested && token.IsCancellationRequested) { + if (!connected) + { + Found(endpointPort, false); + } + // Trace rather than Logging, because this file is linked into the viewer too Trace.WriteLine( $"Timed out after {timeToWait} waiting for the inline queue owner on port {endpointPort}. " + - $"Verb: {message.Verb}. The owner is present but unresponsive. {exception.GetType().Name}"); + $"Verb: {message.Verb}. " + + (connected ? "The owner is present but unresponsive. " : "Nothing accepted the connection. ") + + exception.GetType().Name); return SendOutcome.NoOwner; } // Cancellation is the caller's business; a missing owner is not. catch (Exception exception) when (exception is not OperationCanceledException && Ignorable(exception)) { + // As on the synchronous overload: a connect that failed is an unowned port, and an + // accepted connection that fell over afterwards is not + if (!connected) + { + Found(endpointPort, false); + } + return SendOutcome.NoOwner; } } diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 62dd8cdd..efedfda8 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -128,7 +128,9 @@ public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string ta return LaunchResult.AlreadyRunningAndSupportsRefresh; } - if (ViewerClient.TrySend(new(ViewerVerb.Diff, tempFile, targetFile), out var response)) + // A port recently found unowned is not asked again: the gate below probes for itself + // before launching, and its probe corrects the memory when an owner has arrived since + if (ViewerClient.TrySend(new(ViewerVerb.Diff, tempFile, targetFile), out var response, skipIfUnowned: true)) { return response.Ok ? LaunchResult.AlreadyRunningAndSupportsRefresh @@ -180,7 +182,7 @@ await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFi return LaunchResult.AlreadyRunningAndSupportsRefresh; } - var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Diff, tempFile, targetFile), cancel); + var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Diff, tempFile, targetFile), cancel, skipIfUnowned: true); if (outcome == SendOutcome.Accepted) { return LaunchResult.AlreadyRunningAndSupportsRefresh;