diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 6241272b..62dd8cdd 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -96,9 +96,24 @@ await ViewerLaunchGate.LaunchAsync( /// /// The window is a when the tray took the move, because the /// tray tracks it and the queue owner - normally that same tray - only has to raise something - /// over it. In the arrangement where a viewer owns the queue while a tray runs, that viewer - /// does not know the tray's files, so the focus finds nothing and the pair stays what it was - /// before any of this: an entry in the tray menu. + /// over it. + /// + /// + /// A refused focus falls through to rather than being discarded, + /// because the two sends are two connections and nothing orders them. The piper send is fire + /// and forget: it reports that the bytes went out, not that the move was tracked, and the tray + /// reads that connection on a task of its own - through a solution directory walk, on the first + /// move for a path - while the focus is already asking about a key that has not landed. Focus + /// refuses an unknown key and raises nothing, so losing that race was a first run where the + /// pair reached the tray menu and no window ever opened, and a second run where the same key + /// was still tracked and one did. Diff tracks and raises in a single message to a single + /// process, so there is no order left to get wrong, and its tracking is keyed on the received + /// file like the piper move's, so whichever lands second updates the one entry. + /// + /// + /// It is also the answer in the arrangement where a viewer owns the queue while a tray runs: + /// that viewer does not know the tray's files, so the focus can never find the key. The pair is + /// tracked on both sides there rather than shown by neither. /// /// public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string targetFile) @@ -107,9 +122,9 @@ public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string ta // the tray works out the same two values for itself when a move arrives without them. var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (TrayAvailable && - PiperClient.SendMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null)) + PiperClient.SendMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null) && + ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)))) { - ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile))); return LaunchResult.AlreadyRunningAndSupportsRefresh; } @@ -159,9 +174,9 @@ public static async Task AddDiffAsync(ResolvedTool tool, string te { var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (TrayAvailable && - await PiperClient.SendMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null, cancel)) + await PiperClient.SendMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null, cancel) && + await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel)) { - await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel); return LaunchResult.AlreadyRunningAndSupportsRefresh; } diff --git a/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs b/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs new file mode 100644 index 00000000..fe4baa6f --- /dev/null +++ b/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs @@ -0,0 +1,188 @@ +#pragma warning disable CS0618 // DiffEngineTray is obsolete; the test drives it directly to enable the send path. + +/// +/// A failing pair whose diff tool is the viewer, sent while the tray has taken its move but not yet +/// tracked it. +/// +/// The route sends two messages on two connections: the move to the piper port, then a focus to +/// whoever owns the queue, naming the key that move was tracked under. Nothing orders them. The +/// piper send is fire and forget - it reports that the bytes went out, not that the tray read them +/// - and the tray takes that connection on a task of its own, reads it to the end, and walks up +/// from the target looking for a solution before the entry is visible to anything asking. The focus +/// is already in flight through all of it. +/// +/// +/// Focus refuses a key it cannot find and raises nothing, so losing that race left the pair in the +/// tray menu with no window ever opened - and a second run, where the key was still tracked from +/// the first, opening one. Which is the report this is written from: the diff tool does not show, +/// then on the next run it does. +/// +/// +/// The race is held open rather than raced for: a real takes the move +/// and never gives it to the tracker, which is what the real one looks like for as long as it is +/// still reading. What has to happen anyway is the window, so the assertions are on the owner - a +/// real over a real socket - rather than on which verb got it there. +/// +/// +public class DiffRunnerViewerFocusRaceTest +{ + [Test] + public async Task A_sync_launch_raises_a_window_for_a_move_the_tray_has_not_tracked() => + await AssertRaisesAWindow(_ => Task.FromResult(DiffRunner.Launch(Viewer(), _.Temp, _.Target))); + + [Test] + public async Task An_async_launch_raises_a_window_for_a_move_the_tray_has_not_tracked() => + await AssertRaisesAWindow(_ => DiffRunner.LaunchAsync(Viewer(), _.Temp, _.Target)); + + static async Task AssertRaisesAWindow(Func> launch) + { + await using var fixture = new Fixture(); + + var result = await launch(fixture); + + // The pair has a surface, which is all the caller is ever told. + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + + // The move did go to the tray. It is still in flight there, which is the whole premise: + // the focus that followed it could not find the key, exactly as it cannot while the real + // tray is still reading that connection. + var move = await fixture.PiperMove(); + await Assert.That(move.Temp).IsEqualTo(fixture.Temp); + await Assert.That(move.Target).IsEqualTo(fixture.Target); + + // So the pair went over again as a Diff, which tracks and raises in the one message. + await Assert.That(fixture.Tracks(TrackedKeys.ForMove(fixture.Temp))).IsTrue(); + await Assert.That(fixture.Launches).IsEqualTo(1); + } + + /// + /// An owner that answers the focus for a key it holds still takes the early return, so the + /// fall through is the refusal and not something every pair now pays for. + /// + [Test] + public async Task A_move_the_tray_has_tracked_is_a_focus_and_nothing_more() + { + await using var fixture = new Fixture(); + fixture.Track(); + + var result = DiffRunner.Launch(Viewer(), fixture.Temp, fixture.Target); + + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + await Assert.That(fixture.Launches).IsEqualTo(1); + // Focus raised the window over the entry that was already there, so nothing re-tracked it: + // a Diff would have replaced the move, losing the exe and arguments the piper send carries. + await Assert.That(fixture.TrackedExe()).IsEqualTo(Exe); + } + + sealed class Fixture : + IAsyncDisposable + { + readonly string directory = Path.Combine(Path.GetTempPath(), $"Viewer Focus {Guid.NewGuid():N}"); + readonly OwnedInlineHost host; + readonly RecordingTracker tracker; + readonly FakeLauncher launcher = new(); + readonly CancelSource piperCancel = new(); + readonly Task piper; + readonly TaskCompletionSource move = new(); + readonly bool originalDisabled = DiffRunner.Disabled; + readonly int originalPiperPort = PiperClient.Port; + readonly string? originalViewerPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + + public Fixture() + { + Directory.CreateDirectory(directory); + Temp = Path.Combine(directory, "Sample.Test.received.txt"); + Target = Path.Combine(directory, "Sample.Test.verified.txt"); + File.WriteAllText(Temp, "received"); + + // A real tray listener that takes the move and stops there, which is what the real one + // is doing for as long as it is reading the connection and walking for a solution. + PiperClient.Port = FreePort(); + piper = PiperServer.Start(_ => move.TrySetResult(_), _ => { }, piperCancel.Token); + + host = OwnedInlineHost.TryOwn(_ => { }, launcher, 0) ?? + throw new("Could not bind an ephemeral port."); + // Before the tracker, which builds a RemoteInlineHost of its own and would otherwise + // ask the port the module initializer left pointing at nothing. + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, host.Port.ToString()); + tracker = new(); + host.TrackedFiles = tracker; + host.Start(); + + DiffEngine.DiffEngineTray.IsRunning = true; + DiffRunner.Disabled = false; + } + + public string Temp { get; } + + public string Target { get; } + + public int Launches => launcher.Launches; + + /// + /// The move landing before the focus does, which is the other side of the race and the one + /// that always worked. + /// + public void Track() => + tracker.AddMove(Temp, Target, Exe, "--diff", false, null); + + public bool Tracks(string key) => + ((ITrackedFiles) tracker).Has(key); + + public string? TrackedExe() => + tracker.Moves.Single().Exe; + + public async Task PiperMove() => + await move.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + static int FreePort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + try + { + return ((IPEndPoint) probe.LocalEndpoint).Port; + } + finally + { + probe.Stop(); + } + } + + public async ValueTask DisposeAsync() + { + DiffEngine.DiffEngineTray.IsRunning = false; + DiffRunner.Disabled = originalDisabled; + PiperClient.Port = originalPiperPort; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, originalViewerPort); + + await piperCancel.CancelAsync(); + await piper; + piperCancel.Dispose(); + await host.DisposeAsync(); + await tracker.DisposeAsync(); + Directory.Delete(directory, true); + } + } + + /// + /// Guarded as existing and never started: the pair is the viewer's, so nothing here reaches a + /// process launch. + /// + static readonly string Exe = Environment.ProcessPath!; + + static ResolvedTool Viewer() => + new( + name: DiffTool.DiffEngineViewer.ToString(), + tool: DiffTool.DiffEngineViewer, + exePath: Exe, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + isMdi: false, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); +} diff --git a/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs index bceafc9c..eb71de3c 100644 --- a/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs +++ b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs @@ -149,7 +149,15 @@ static int GetFreePort() readonly string temp; readonly string target; readonly bool originalDisabled = DiffRunner.Disabled; - readonly string? originalViewerPort = Environment.GetEnvironmentVariable("DiffEngine_ViewerPort"); + + /// + /// An owner that answers the focus the route sends once the tray has taken the move, and that + /// publishes its own port so a live viewer on this machine is not raised. A port with nothing + /// on it would leave the focus unanswered, which is the fall through to + /// that is about; + /// this one is about what rides the move. + /// + readonly FakeViewer owner = new(); public DiffRunnerViewerMoveTest() { @@ -158,9 +166,6 @@ public DiffRunnerViewerMoveTest() target = Path.Combine(directory, "Sample.Test.verified.txt"); File.WriteAllText(temp, "received"); PiperClient.Port = GetFreePort(); - // The route sends a focus to whoever owns the queue after the tray has taken the move. - // Pointed at a free port so a live viewer on the machine running these is not raised. - Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", GetFreePort().ToString()); DiffEngine.DiffEngineTray.IsRunning = true; DiffRunner.Disabled = false; } @@ -169,7 +174,7 @@ public void Dispose() { DiffEngine.DiffEngineTray.IsRunning = false; DiffRunner.Disabled = originalDisabled; - Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", originalViewerPort); + owner.Dispose(); Directory.Delete(directory, true); } }