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