From 8d41eb2a02731fe8d1ab7ea750609624e52d0f95 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Mon, 31 Aug 2026 15:24:45 +1000 Subject: [PATCH] Open the tray menu without a socket round trip 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. --- .../TrayViewerSyncTest.cs | 70 ++++++++++++++----- src/DiffEngineTray/RemoteInlineHost.cs | 65 +++++++++++++++-- src/DiffEngineTray/Tracker.cs | 59 ++++++++++------ 3 files changed, 145 insertions(+), 49 deletions(-) diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 54be2074..cfb1a51c 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/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)]); } /// @@ -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)]); } /// @@ -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(); } /// @@ -821,6 +821,24 @@ public TrayOwned(Func? applier = null) public List Warnings { get; } = []; public List Failures { get; } = []; + /// + /// What the tray menu is built from. is the last listing + /// seen rather than a live read, so this refreshes first - standing in for the two second + /// scan, and for where that is what keeps it current. + /// + /// Reading the property alone would assert against whatever the cache happened to hold, + /// which for an empty expectation is a test that cannot fail. + /// + /// + public IReadOnlyList Listing + { + get + { + Tracker.Refresh(); + return Tracker.Snapshots; + } + } + readonly string root = TempRoot(); /// @@ -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; + /// + /// What the tray menu is built from, refreshed first - see + /// . It matters more on this side: with the queue in the + /// other process, the cache only moves when something refreshes it. + /// + public IReadOnlyList 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) diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index f89c8152..288bd817 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -1,17 +1,24 @@ +using System.Net.NetworkInformation; + /// /// 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. /// -/// 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 - so nothing here is waited on +/// from the UI thread except a verb the user clicked. /// /// /// Accepting does not, for the reason gives. /// /// -/// 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. +/// +/// +/// "No owner" is asked of the OS rather than found out by connecting - see +/// . /// /// 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); + } + + /// + /// Whether anything holds the port, asked of the OS rather than found out by connecting to it. + /// + /// 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 + /// 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. + /// + /// + /// 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. + /// + /// + 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; + } + } } diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index b18f0141..9b762d52 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -10,8 +10,9 @@ class Tracker : ConcurrentDictionary moves = new(StringComparer.OrdinalIgnoreCase); ConcurrentDictionary 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 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); } }); - } /// /// Accepts just these snapshots, for a group header: unlike , @@ -936,17 +942,24 @@ int ITrackedFiles.DiscardAll() } /// - /// 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. + /// + /// This is what the menu is built from, and building it runs on the UI thread inside + /// ContextMenuStrip.Opening. 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 + /// , so every menu open took half a second for the rest + /// of the tray's life. + /// + /// + /// Nothing is lost where the queue is held here: runs + /// 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 and the icon have always shown. + /// /// - public IReadOnlyList Snapshots - { - get - { - snapshots = inline.List(); - return snapshots; - } - } + public IReadOnlyList Snapshots => snapshots; /// /// Deliberately not : exiting is not discarding. The diff tools this tray