Open the tray menu without a socket round trip (#860)

Right clicking the tray icon took half a second whenever a viewer owned the
inline queue and had since exited. The menu is built on the UI thread inside
ContextMenuStrip.Opening, and BuildTrackingMenuItems read Tracker.Snapshots,
which read the queue live - a loopback exchange bounded by
ViewerClient.ShortTimeout. Measured from the shell's own notify icon message to
the menu being open: 3-26ms with the queue held here, 513ms without, and on
every click rather than the first.

Snapshots is the scan cache now. Nothing is lost where the queue is held here,
since OwnedInlineHost.Changed already runs Refresh on every mutation and the
tray's own accepts and discards refresh too, so the cache is the live queue.
Where a viewer holds it the listing is at most one scan old, which is what
TrackingAny and the icon have always shown - and what MenuBuilder already said
it was showing. The Tracker seeds the cache in its constructor rather than
leaving it empty until the first scan two seconds later, so a tray that has just
started does not show an empty menu over a queue that is not.

AcceptAllSnapshots keeps its live read, because that guard exists precisely so a
stale empty cache cannot make it silently do nothing, but takes it inside the
worker rather than in front of it: the caller is a click or a hot key and the
read is a round trip.

The half second itself is the other half of this. A connection to a port nothing
is listening on is supposed to be refused at once, and every caller in
RemoteInlineHost was written expecting it - "a refused connection means the
viewer has gone". It is not refused at once everywhere: where the SYN is dropped
rather than answered with a reset, the connect runs to its timeout instead. On
one machine a closed loopback port costs 503ms against a 500ms cap, and 2034ms
uncapped, the same for a dual mode socket and an IPv4 one. Ownership is decided
at startup and this host is never replaced, so that was the price of every scan
and every menu verb for the rest of the tray's life.

Exchange asks the OS whether anything holds the port before connecting. The
listener table is a local query costing well under a millisecond, and it turns
the gone owner case from 513ms into 1.1ms. Cheaper than a backoff and with no
staleness window: a viewer that starts is found on the next call, and racing the
check costs no more than the connect always did.

TrayViewerSyncTest's two pair helpers gained a Listing that refreshes before
reading, standing in for the scan timer. That is also what
ViewerAcceptAllEmptiesTheTrayListing needed to be a test at all: it asserts an
empty listing, and a cache satisfies that whether or not anything worked.

Author: SimonCropp

src/DiffEngineTray.Tests/TrayViewerSyncTest.cs

@@ -79,7 +79,7 @@ public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheAttachedViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -160,7 +160,7 @@ public async Task ASettleReachesTheAttachedViewer()
         pair.Send(new(ViewerVerb.Settle, Key(sample, 1)));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     /// <summary>
@@ -181,7 +181,7 @@ public async Task ViewerAcceptAllEmptiesTheTray()
 
         var viewer = pair.Pump();
         await Assert.That(viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Tracker.Moves).IsEmpty();
         await Assert.That(pair.Tracker.Deletes).IsEmpty();
         await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received");
@@ -199,7 +199,7 @@ public async Task ViewerAcceptOfOneSnapshotReachesTheTray()
         pair.Link.Post(ViewerSideVerb.Accept, Key(sample, 1));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]);
     }
 
@@ -214,7 +214,7 @@ public async Task ViewerDiscardOfOneSnapshotReachesTheTray()
         pair.Link.Post(ViewerSideVerb.Discard, Key(sample, 1));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -270,7 +270,7 @@ public async Task AcceptingASnapshotThatIsAlreadyGoneSaysNothing()
 
         await Assert.That(pair.Failures).IsEmpty();
         await Assert.That(pair.Applied).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     /// <summary>
@@ -289,7 +289,7 @@ public async Task AFailedAcceptStaysPendingOnBothSides()
         var viewer = pair.Pump();
         await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(viewer.Exit).IsFalse();
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -314,7 +314,7 @@ public async Task TrayAcceptAllReportsWhatStayedPending()
         var viewer = pair.Pump();
         await Assert.That(viewer.Keys()).IsEquivalentTo([Key(sample, 1)]);
         await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -357,7 +357,7 @@ public async Task TrayAcceptAllEmptiesTheOwningViewer()
         await pair.Tracker.AcceptAll();
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Applied.Count).IsEqualTo(2);
     }
 
@@ -389,7 +389,7 @@ public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheOwningViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -415,7 +415,7 @@ public async Task TrayDiscardAllEmptiesTheOwningViewer()
         pair.Tracker.Clear();
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -429,7 +429,7 @@ public async Task ViewerAcceptReachesTheTrayListing()
         pair.Act(CommandKind.Accept, Key(sample, 1));
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -442,7 +442,7 @@ public async Task ViewerAcceptAllEmptiesTheTrayListing()
         pair.Act(CommandKind.AcceptAll, null);
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Tracker.TrackingAny).IsFalse();
     }
 
@@ -456,7 +456,7 @@ public async Task ViewerDiscardReachesTheTrayListing()
         pair.Act(CommandKind.Discard, Key(sample, 1));
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -473,7 +473,7 @@ public async Task AFailedAcceptStaysPendingOnBothSidesOfAnOwningViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -492,7 +492,7 @@ public async Task TrayAcceptAllReportsWhatTheOwningViewerKept()
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1)]);
         await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -518,7 +518,7 @@ public async Task ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer()
 
         await Assert.That(pair.Failures).IsEmpty();
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
     }
 
     /// <inheritdoc cref="ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer"/>
@@ -821,6 +821,24 @@ public TrayOwned(Func<InlinePatch, InlineApplyResult>? applier = null)
         public List<string> Warnings { get; } = [];
         public List<string> Failures { get; } = [];
 
+        /// <summary>
+        /// What the tray menu is built from. <see cref="Tracker.Snapshots"/> is the last listing
+        /// seen rather than a live read, so this refreshes first - standing in for the two second
+        /// scan, and for <see cref="OwnedInlineHost.Changed"/> where that is what keeps it current.
+        /// <para>
+        /// Reading the property alone would assert against whatever the cache happened to hold,
+        /// which for an empty expectation is a test that cannot fail.
+        /// </para>
+        /// </summary>
+        public IReadOnlyList<PendingSnapshot> Listing
+        {
+            get
+            {
+                Tracker.Refresh();
+                return Tracker.Snapshots;
+            }
+        }
+
         readonly string root = TempRoot();
 
         /// <summary>
@@ -858,7 +876,7 @@ public PendingSnapshot Queue(string source, int line, string content = "new", st
                 throw new($"The owner refused the patch. {response.Message}");
             }
 
-            return Tracker.Snapshots.Single(_ => _.Key == Key(source, line));
+            return Listing.Single(_ => _.Key == Key(source, line));
         }
 
         public TrackedMoveFiles AddMove()
@@ -982,6 +1000,20 @@ public TrackedMoveFiles StageMove()
 
         public SessionState Viewer => Window.State;
 
+        /// <summary>
+        /// What the tray menu is built from, refreshed first - see
+        /// <see cref="TrayOwned.Listing"/>. It matters more on this side: with the queue in the
+        /// other process, the cache only moves when something refreshes it.
+        /// </summary>
+        public IReadOnlyList<PendingSnapshot> Listing
+        {
+            get
+            {
+                Tracker.Refresh();
+                return Tracker.Snapshots;
+            }
+        }
+
         public void Queue(string source, int line, string content = "new", string? framework = null)
         {
             var message = new ViewerMessage(ViewerVerb.Inline, Body: Payload(source, line, content, framework));
@@ -995,7 +1027,7 @@ public void Queue(string source, int line, string content = "new", string? frame
         public PendingSnapshot Snapshot(string source, int line, string content = "new", string? framework = null)
         {
             Queue(source, line, content, framework);
-            return Tracker.Snapshots.Single(_ => _.Key == Key(source, line));
+            return Listing.Single(_ => _.Key == Key(source, line));
         }
 
         public ViewerResponse Send(ViewerMessage message)

src/DiffEngineTray/RemoteInlineHost.cs

@@ -1,17 +1,24 @@
+using System.Net.NetworkInformation;
+
 /// <summary>
 /// The queue belongs to a viewer that bound the port before this tray started, so every call is a
 /// short loopback round trip and the tray is a remote control.
 /// <para>
-/// The listing and the menu verbs use ViewerClient.ShortTimeout. Those run from the 2 second scan
-/// timer and from the menu opening, so a slow exchange must not outlast the timer period or block
-/// the UI.
+/// The listing and the menu verbs use ViewerClient.ShortTimeout. The listing runs from the 2
+/// second scan timer, so a slow exchange must not outlast the timer period. It is no longer what
+/// the menu is built from - see <see cref="Tracker.Snapshots"/> - so nothing here is waited on
+/// from the UI thread except a verb the user clicked.
 /// </para>
 /// <para>
 /// Accepting does not, for the reason <see cref="acceptWait"/> gives.
 /// </para>
 /// <para>
-/// A refused connection means the viewer has gone, which is the same as nothing pending. The queue
-/// went with it, and this tray does not take ownership: it was decided at startup.
+/// No owner means the viewer has gone, which is the same as nothing pending. The queue went with
+/// it, and this tray does not take ownership: it was decided at startup.
+/// </para>
+/// <para>
+/// "No owner" is asked of the OS rather than found out by connecting - see
+/// <see cref="PortIsHeld"/>.
 /// </para>
 /// </summary>
 class RemoteInlineHost : IInlineHost
@@ -156,6 +163,50 @@ static bool Send(ViewerVerb verb, string? key, TimeSpan wait, out string? messag
         return response.Ok;
     }
 
-    static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response) =>
-        ViewerClient.TrySend(message, out response, wait: wait);
+    static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response)
+    {
+        if (!PortIsHeld())
+        {
+            response = null;
+            return false;
+        }
+
+        return ViewerClient.TrySend(message, out response, wait: wait);
+    }
+
+    /// <summary>
+    /// Whether anything holds the port, asked of the OS rather than found out by connecting to it.
+    /// <para>
+    /// Connecting to a port nothing is listening on is supposed to be refused at once, and every
+    /// caller here was written expecting that. It is not refused at once on every machine: where
+    /// the SYN is dropped rather than answered with a reset, the connect runs to its timeout
+    /// instead. Once the owning viewer exits, that is the full
+    /// <see cref="ViewerClient.ShortTimeout"/> per call - half a second on the two second scan,
+    /// and half a second on every menu verb - for the rest of this tray's life, because ownership
+    /// is decided at startup and this host is never replaced.
+    /// </para>
+    /// <para>
+    /// The listener table is a local kernel query costing well under a millisecond, and it answers
+    /// the only question worth asking first. Matched on the port alone: a listener on any address
+    /// accepts a loopback connection, so the round trip is skipped only when nothing at all holds
+    /// the port. Racing it is harmless either way - an owner that binds just after the check is
+    /// found by the next call, and one that exits just after it costs the timeout exactly as
+    /// before.
+    /// </para>
+    /// </summary>
+    static bool PortIsHeld()
+    {
+        var port = ViewerClient.Port;
+        try
+        {
+            return IPGlobalProperties.GetIPGlobalProperties()
+                .GetActiveTcpListeners()
+                .Any(_ => _.Port == port);
+        }
+        catch (NetworkInformationException)
+        {
+            // No table to read, so let the connect decide as it always did
+            return true;
+        }
+    }
 }

src/DiffEngineTray/Tracker.cs

@@ -10,8 +10,9 @@ class Tracker :
     ConcurrentDictionary<string, TrackedMove> moves = new(StringComparer.OrdinalIgnoreCase);
     ConcurrentDictionary<string, TrackedDelete> deletes = new(StringComparer.OrdinalIgnoreCase);
     IInlineHost inline;
-    // The last listing seen, used for the icon state. Free when this tray owns the queue, and a
-    // loopback round trip when a viewer does, which is why the menu re-reads live when it opens.
+    // The last listing seen, used for the icon state and for the menu. Free when this tray owns
+    // the queue, and a loopback round trip when a viewer does, which is why nothing re-reads it
+    // from a click.
     IReadOnlyList<PendingSnapshot> snapshots = [];
     AsyncTimer timer;
     int lastScanCount;
@@ -31,6 +32,11 @@ public Tracker(Action active, Action inactive, LockedFilesResolver? lockedFilesR
             {
                 ExceptionHandler.Handle("Failed to scan files", exception);
             });
+
+        // Seeded rather than left empty until the first scan two seconds later. The menu reads
+        // this cache now, so without it a tray that has just started shows none of what a viewer
+        // already had queued - and the icon stays dark for the same two seconds.
+        Refresh();
     }
 
     Task ScanFiles(Cancel cancel)
@@ -316,19 +322,20 @@ public Task Discard(PendingSnapshot snapshot) =>
             }
         });
 
-    public Task AcceptAllSnapshots()
-    {
-        // Live read, not the scan cache: this can be called before the first scan, and acting on
-        // a stale empty cache would silently do nothing.
-        if (Snapshots.Count == 0)
-        {
-            return Task.CompletedTask;
-        }
-
-        return Task.Run(() =>
+    public Task AcceptAllSnapshots() =>
+        Task.Run(() =>
         {
             try
             {
+                // Live read, not the scan cache: this can be called before the first scan, and
+                // acting on a stale empty cache would silently do nothing. Inside the worker
+                // rather than in front of it, because the caller is a menu click or a hot key and
+                // the read is a round trip whenever a viewer owns the queue.
+                if (inline.List().Count == 0)
+                {
+                    return;
+                }
+
                 if (!inline.AcceptAll(out var message))
                 {
                     inlineFailed?.Invoke($"Could not accept the pending snapshots. {message}");
@@ -341,7 +348,6 @@ public Task AcceptAllSnapshots()
                 ExceptionHandler.Handle("Failed to accept the pending snapshots", exception);
             }
         });
-    }
 
     /// <summary>
     /// Accepts just these snapshots, for a group header: unlike <see cref="AcceptAllSnapshots"/>,
@@ -936,17 +942,24 @@ int ITrackedFiles.DiscardAll()
     }
 
     /// <summary>
-    /// Read live rather than from the scan cache, so the menu shows the viewer's current queue at
-    /// the moment it opens.
+    /// The last listing seen, rather than a fresh one.
+    /// <para>
+    /// This is what the menu is built from, and building it runs on the UI thread inside
+    /// <c>ContextMenuStrip.Opening</c>. Reading live there put a loopback round trip between the
+    /// right click and the menu whenever a viewer owned the queue. Worse, a connection to a port
+    /// nothing is listening on is only refused at once on some machines - where the SYN is dropped
+    /// instead, an owner that had exited cost the whole of
+    /// <see cref="ViewerClient.ShortTimeout"/>, so every menu open took half a second for the rest
+    /// of the tray's life.
+    /// </para>
+    /// <para>
+    /// Nothing is lost where the queue is held here: <see cref="OwnedInlineHost.Changed"/> runs
+    /// <see cref="Refresh"/> on every mutation, and the tray's own accepts and discards refresh
+    /// too, so the cache is the live queue. Where a viewer holds it, the listing is at most one
+    /// scan old - which is what <see cref="TrackingAny"/> and the icon have always shown.
+    /// </para>
     /// </summary>
-    public IReadOnlyList<PendingSnapshot> Snapshots
-    {
-        get
-        {
-            snapshots = inline.List();
-            return snapshots;
-        }
-    }
+    public IReadOnlyList<PendingSnapshot> Snapshots => snapshots;
 
     /// <summary>
     /// Deliberately not <see cref="Clear"/>: exiting is not discarding. The diff tools this tray