diff --git a/claude.md b/claude.md index e7659518..e353b76b 100644 --- a/claude.md +++ b/claude.md @@ -152,6 +152,18 @@ apart. Whether an entry is hidden is always read back out of `VisibleEntries`, never recomputed — the rules about when a header exists at all live in one place and must stay there. A fold is a view: `AcceptAll` still sweeps what it hides, which `CollapseTests` pins. +- Accept-all goes an entry at a time, because it takes as long as the queue is long. + `ViewerSession.BeginAcceptAll` records an `AcceptBatch`, and `AcceptAllRunner` claims an entry + under `SessionHost`'s lock (`ClaimNext`), applies it outside (`ApplyClaimed`), and records it + under the lock again - snapshots before files, since whether a delete is held turns on how the + snapshots went. The render loop takes that lock every frame, so one transition over the queue + froze the window for the whole batch. A window's batch runs on a worker, a wire `AcceptAll` on + its listener thread, and `ViewerSession.Apply(AcceptAll)` is the same steps back to back, which + is what the tests drive. Owners put `AcceptProgress` on their listings - the tray completes each + snapshot with `InlineQueue.AcceptInBatch` rather than all at the end - and `OwnerLink.Run` lists + beside an in-flight send rather than after it, so an attached window follows the owner's batch. + While `SessionState.Progress` is set the status line shows it and the window refuses anything + `ChangesQueue` names. - Images (`Images/`, extensions in `DiffEngine/Viewer/ImageExtensions.cs`, linked into the viewer so the tool registration and the renderer cannot disagree) are a side, not a mode. `FileSide.Read` decides text or picture **by extension**, because the expected side of a new snapshot has no bytes diff --git a/docs/mdsource/tray.source.md b/docs/mdsource/tray.source.md index 6ad0aeb9..626a5cfb 100644 --- a/docs/mdsource/tray.source.md +++ b/docs/mdsource/tray.source.md @@ -59,6 +59,8 @@ Exiting the tray writes any still-pending inline snapshots back to disk, under t "Accept all" will accept all pending moves, deletes and inline snapshots. Snapshots whose target frameworks disagree about the content are skipped rather than picked between; resolve those in the viewer. +A long queue takes a while to accept. An open [DiffEngineViewer](/docs/viewer.md) window shows how far it has got, with each snapshot leaving the list as it lands. + ### Locked files diff --git a/docs/mdsource/viewer.source.md b/docs/mdsource/viewer.source.md index 45b35c2d..0caee6f5 100644 --- a/docs/mdsource/viewer.source.md +++ b/docs/mdsource/viewer.source.md @@ -94,6 +94,8 @@ What lands on the clipboard is what is on screen: tabs already expanded to the f A test run that fails several inline snapshots produces one window, not several. Whichever process binds the loopback port holds the queue; everything else hands its patch to that one. The window lists everything pending and offers **Accept all**. +**Accept all** takes as long as the queue is long, so it goes one entry at a time. Each entry leaves the list as it lands, the status line says how far it has got (`Accepting 12 of 40`), and the window keeps responding throughout. **Accept**, **Discard** and **Accept all** are disabled until it finishes; scrolling, selecting and copying are not. It is the same when [DiffEngineTray](/docs/tray.md) holds the queue, and when the accept-all was started from the tray's menu: the window follows the tray's progress. + Failing file comparisons join the same queue, so a run that fails ten snapshots opens one window whether they are inline or on disk. Every other diff tool gets a process per pair, and DiffEngine closes each one as its test starts passing; the viewer is told to drop that row instead. Rows that came from files follow those files. A re-run that rewrites a received file shows the rewrite, a verified file that appears fills in the other pane, and a row whose received file goes away leaves with it — so nothing is offered for a file that is no longer there, however it went. The window closes once the last row does. diff --git a/docs/tray.md b/docs/tray.md index dfb025a3..7cae7824 100644 --- a/docs/tray.md +++ b/docs/tray.md @@ -66,6 +66,8 @@ Exiting the tray writes any still-pending inline snapshots back to disk, under t "Accept all" will accept all pending moves, deletes and inline snapshots. Snapshots whose target frameworks disagree about the content are skipped rather than picked between; resolve those in the viewer. +A long queue takes a while to accept. An open [DiffEngineViewer](/docs/viewer.md) window shows how far it has got, with each snapshot leaving the list as it lands. + ### Locked files diff --git a/docs/viewer.md b/docs/viewer.md index 485cad47..7806896f 100644 --- a/docs/viewer.md +++ b/docs/viewer.md @@ -101,6 +101,8 @@ What lands on the clipboard is what is on screen: tabs already expanded to the f A test run that fails several inline snapshots produces one window, not several. Whichever process binds the loopback port holds the queue; everything else hands its patch to that one. The window lists everything pending and offers **Accept all**. +**Accept all** takes as long as the queue is long, so it goes one entry at a time. Each entry leaves the list as it lands, the status line says how far it has got (`Accepting 12 of 40`), and the window keeps responding throughout. **Accept**, **Discard** and **Accept all** are disabled until it finishes; scrolling, selecting and copying are not. It is the same when [DiffEngineTray](/docs/tray.md) holds the queue, and when the accept-all was started from the tray's menu: the window follows the tray's progress. + Failing file comparisons join the same queue, so a run that fails ten snapshots opens one window whether they are inline or on disk. Every other diff tool gets a process per pair, and DiffEngine closes each one as its test starts passing; the viewer is told to drop that row instead. Rows that came from files follow those files. A re-run that rewrites a received file shows the rewrite, a verified file that appears fills in the other pane, and a row whose received file goes away leaves with it — so nothing is offered for a file that is no longer there, however it went. The window closes once the last row does. diff --git a/src/DiffEngine.Tests/InlineQueueTests.cs b/src/DiffEngine.Tests/InlineQueueTests.cs index 6af4a60f..0f250ea1 100644 --- a/src/DiffEngine.Tests/InlineQueueTests.cs +++ b/src/DiffEngine.Tests/InlineQueueTests.cs @@ -399,6 +399,52 @@ public async Task ABatchCompletionSkipsANewcomer() await Assert.That(message).IsEqualTo("Accepted 2"); } + /// + /// A batch completed an entry at a time, the way an owner applying a long queue completes it + /// so the queue can be watched shrinking. Each step leaves the queue a listing would show at + /// that point, and the steps together say what the whole batch would have. + /// + [Test] + public async Task ABatchCompletedAnEntryAtATimeShrinksAsItGoes() + { + var queue = InlineQueue.Empty + .Enqueue(Patch("A.cs", 1)) + .Enqueue(Patch("B.cs", 2)) + .Enqueue(Patch("C.cs", 3)); + var pending = queue.Items; + var tally = new AcceptAllTally(); + + queue = queue.AcceptInBatch(pending[0], InlineApplyResult.Applied, ref tally); + await Assert.That(queue.Items.Select(_ => _.Name)).IsEquivalentTo(["B.cs:2", "C.cs:3"]); + + queue = queue.AcceptInBatch(pending[1], InlineApplyResult.NotFound("no Verify or Throws call"), ref tally); + queue = queue.AcceptInBatch(pending[2], InlineApplyResult.Applied, ref tally); + + // The stale one stays, as it does out of a whole batch + await Assert.That(queue.Items.Single().Status).IsEqualTo("B.cs:2 not written. no Verify or Throws call"); + await Assert.That(tally.Refused).IsTrue(); + await Assert.That(tally.Message(queue.Conflicts)) + .IsEqualTo("Accepted 2, 1 not written. B.cs:2 not written. no Verify or Throws call"); + } + + /// + /// A re-run that replaced the entry while its patch applied keeps its new content, and the + /// batch does not count an outcome that describes content no longer pending. + /// + [Test] + public async Task ABatchStepSkipsAnEntryReplacedWhileItApplied() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "first")); + var entry = queue.Items.Single(); + queue = queue.Enqueue(Patch(content: "second")); + var tally = new AcceptAllTally(); + + var after = queue.AcceptInBatch(entry, InlineApplyResult.Applied, ref tally); + + await Assert.That(after).IsSameReferenceAs(queue); + await Assert.That(tally).IsEqualTo(new AcceptAllTally()); + } + [Test] public async Task DiscardRemovesWithoutApplying() { diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 7ebdbe6e..90c9cb24 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -608,6 +608,56 @@ public async Task AListingWithNoWindowCommandSaysNothing() await Assert.That(parsed!.Window).IsNull(); } + /// + /// How a viewer displaying someone else's queue learns how far the owner's accept-all has got: + /// on a listing taken while it runs, since the accept itself is one exchange that answers only + /// once the batch is done. + /// + [Test] + public async Task AcceptProgressRidesOnAListing() + { + var text = ViewerResponse.Listing([], progress: new(3, 40)).Build(); + + await Assert.That(text).Contains("progress: 3|40\n"); + await Assert.That(ViewerResponse.TryParse(text, out var parsed)).IsTrue(); + await Assert.That(parsed!.Progress).IsEqualTo(new AcceptProgress(3, 40)); + } + + [Test] + public async Task AListingWithNoAcceptRunningSaysNothingOfProgress() + { + var text = ViewerResponse.Listing([]).Build(); + + await Assert.That(text).DoesNotContain("progress:"); + await Assert.That(ViewerResponse.TryParse(text, out var parsed)).IsTrue(); + await Assert.That(parsed!.Progress).IsNull(); + } + + /// + /// A progress line is only a count, so one that does not parse is a response that does not, + /// the way a malformed move line is. + /// + [Test] + public async Task AMalformedProgressLineRejectsTheResponse() + { + var text = ViewerResponse.Listing([], progress: new(3, 40)).Build() + .Replace("progress: 3|40\n", "progress: 3\n"); + + await Assert.That(ViewerResponse.TryParse(text, out _)).IsFalse(); + } + + /// + /// The entry being worked on rather than the count finished: the first is "1 of 40" while it + /// is applying, and the last is never "41 of 40". + /// + [Test] + public async Task ProgressNamesTheEntryInHand() + { + await Assert.That(new AcceptProgress(0, 40).Describe()).IsEqualTo("Accepting 1 of 40"); + await Assert.That(new AcceptProgress(39, 40).Describe()).IsEqualTo("Accepting 40 of 40"); + await Assert.That(new AcceptProgress(40, 40).Describe()).IsEqualTo("Accepting 40 of 40"); + } + /// /// The client's three second default is what a real caller uses to decide the owner has died. /// The tests below are about what the owner answers rather than how fast, and CI starts six diff --git a/src/DiffEngine/Inline/AcceptAllTally.cs b/src/DiffEngine/Inline/AcceptAllTally.cs new file mode 100644 index 00000000..58c5202e --- /dev/null +++ b/src/DiffEngine/Inline/AcceptAllTally.cs @@ -0,0 +1,27 @@ +namespace DiffEngine; + +/// +/// How a bulk accept has gone so far, counted an entry at a time. +/// +/// Carried between the entries of a batch that completes each one as it lands, rather than all of +/// them at the end, so what the batch reports once it is done is the sentence +/// has always built for a bulk accept. A value rather +/// than something the batch updates in place, because the viewer keeps it in a session state that +/// is immutable. +/// +/// +readonly record struct AcceptAllTally(int Accepted, int NotWritten, int Failed, string? Failure) +{ + /// + /// A patch in this batch that was not written. What holds a sweep's pending deletes, since one + /// of them may be the only copy left of a snapshot that never made it into the source. + /// + public bool Refused => NotWritten + Failed > 0; + + /// + /// Conflicted entries are counted by the caller at the end rather than here, because a batch + /// never applies one: they are whatever the queue still holds with more than one variant. + /// + public string Message(int conflicted) => + InlineQueue.AcceptAllMessage(Accepted, NotWritten, Failed, conflicted, Failure); +} diff --git a/src/DiffEngine/Inline/InlineQueue.cs b/src/DiffEngine/Inline/InlineQueue.cs index cc434ce4..97e4f3e6 100644 --- a/src/DiffEngine/Inline/InlineQueue.cs +++ b/src/DiffEngine/Inline/InlineQueue.cs @@ -464,55 +464,82 @@ public InlineQueue AcceptAll( IReadOnlyList<(PendingInline Entry, InlineApplyResult Result)> outcomes, out string message) { - var remaining = new List(); - var accepted = 0; - var notWritten = 0; - var failed = 0; - var conflicted = 0; - string? failure = null; - foreach (var entry in Items) + var queue = this; + var tally = new AcceptAllTally(); + foreach (var (entry, result) in outcomes) { - var outcome = outcomes.FirstOrDefault(_ => ReferenceEquals(_.Entry.Variants, entry.Variants)); - if (outcome.Entry is null) - { - if (entry.Conflicted) - { - conflicted++; - } + queue = queue.AcceptInBatch(entry, result, ref tally); + } - remaining.Add(entry); - continue; - } + message = tally.Message(queue.Conflicts); + return queue; + } - var (removed, stale, text) = Outcome(entry, outcome.Result); - // Dropped on its own, an entry the reader was watching and got an answer about. Dropped - // out of a batch of thirty, an entry nobody saw go: no literal written, nothing left in - // the queue to say so, and a count of accepts that included it. So it stays, carrying - // what the applier said, the way every other unwritten snapshot in the batch does. A - // re-run brings the patch back and the arrival clears the status. - if (stale) - { - notWritten++; - failure = text; - remaining.Add(entry with { Status = text }); - continue; - } + /// + /// One entry of a bulk accept, completed on its own rather than with the rest of the batch. + /// + /// A host applying a long queue commits each outcome as it arrives, so whoever is watching the + /// queue sees it shrink as the batch goes, and a listing taken partway through says how far it + /// has got. Completing everything at the end left a window showing an untouched queue for as + /// long as the batch took, and then emptying all at once. + /// + /// + /// The rules are the batch's rather than a single accept's, because the batch completion above + /// is this, once per outcome. An entry that changed while its patch was applying is left alone + /// and not counted, found by its variants the way the two phase accept finds it. + /// + /// + internal InlineQueue AcceptInBatch(PendingInline entry, InlineApplyResult result, ref AcceptAllTally tally) + { + var items = Items.ToList(); + var index = items.FindIndex(_ => ReferenceEquals(_.Variants, entry.Variants)); + if (index < 0) + { + return this; + } - if (removed) + var current = items[index]; + var (removed, stale, text) = Outcome(current, result); + // Dropped on its own, an entry the reader was watching and got an answer about. Dropped + // out of a batch of thirty, an entry nobody saw go: no literal written, nothing left in + // the queue to say so, and a count of accepts that included it. So it stays, carrying + // what the applier said, the way every other unwritten snapshot in the batch does. A + // re-run brings the patch back and the arrival clears the status. + if (stale) + { + tally = tally with { - accepted++; - continue; - } - - failed++; - failure = text; - remaining.Add(entry with { Status = text }); + NotWritten = tally.NotWritten + 1, + Failure = text + }; + items[index] = current with { Status = text }; + } + else if (removed) + { + tally = tally with { Accepted = tally.Accepted + 1 }; + items.RemoveAt(index); + } + else + { + tally = tally with + { + Failed = tally.Failed + 1, + Failure = text + }; + items[index] = current with { Status = text }; } - message = AcceptAllMessage(accepted, notWritten, failed, conflicted, failure); - return new(remaining); + return new(items); } + /// + /// What a bulk accept that has finished counts as still needing review: it never applies an + /// entry with more than one variant, so whatever the queue holds of those is left for a + /// reviewer to pick from. + /// + internal int Conflicts => + Items.Count(_ => _.Conflicted); + public PendingInline? Find(string key) => Items.FirstOrDefault(_ => _.Key == key); diff --git a/src/DiffEngine/Protocol/AcceptProgress.cs b/src/DiffEngine/Protocol/AcceptProgress.cs new file mode 100644 index 00000000..ccc45811 --- /dev/null +++ b/src/DiffEngine/Protocol/AcceptProgress.cs @@ -0,0 +1,51 @@ +namespace DiffEngine; + +/// +/// How far an accept-all has got: of the entries +/// it set out to deal with, whichever way each of them went. +/// +/// Accepting a long queue takes as long as the queue is long - every snapshot is a read, a parse +/// and a write under a cross process mutex, and every move can be retried for seconds while a diff +/// tool lets go of it - and nothing said so. A window that owned the queue froze for the whole +/// batch, and one displaying someone else's said "Waiting for the queue owner." over a list that +/// did not move until everything went at once. +/// +/// +/// On the wire because the batch runs in whichever process owns the queue and the window may +/// belong to another one. The owner answers a listing taken partway through with this, and the +/// displaying viewer says it in the same words the owning one would. +/// +/// +record AcceptProgress(int Done, int Total) +{ + /// + /// The entry being worked on rather than the count finished, which is how a progress line + /// reads: the first entry is "1 of 40" while it is being applied, not "0 of 40". + /// + public string Describe() => + $"Accepting {Math.Min(Done + 1, Total)} of {Total}"; + + /// + /// One more entry dealt with, however it went. + /// + public AcceptProgress Advance() => + this with { Done = Done + 1 }; + + public string Build() => + $"{Done}|{Total}"; + + public static bool TryParse(string value, [NotNullWhen(true)] out AcceptProgress? progress) + { + progress = null; + var parts = value.Split('|'); + if (parts.Length != 2 || + !int.TryParse(parts[0], out var done) || + !int.TryParse(parts[1], out var total)) + { + return false; + } + + progress = new(done, total); + return true; + } +} diff --git a/src/DiffEngine/Protocol/ViewerResponse.cs b/src/DiffEngine/Protocol/ViewerResponse.cs index ea988191..d19ee9ba 100644 --- a/src/DiffEngine/Protocol/ViewerResponse.cs +++ b/src/DiffEngine/Protocol/ViewerResponse.cs @@ -65,6 +65,13 @@ record ViewerResponse( public IReadOnlyList Deletes { get; init; } = []; + /// + /// How far the accept-all the owner is running has got, on a listing taken while one is, and + /// null the rest of the time. A reader that predates it skips the line, as it skips any name it + /// does not know. + /// + public AcceptProgress? Progress { get; init; } + public static ViewerResponse Success(string? message = null) => new(true, message, []); @@ -76,11 +83,13 @@ public static ViewerResponse Listing( WindowCommand? window = null, string? windowKey = null, IReadOnlyList? moves = null, - IReadOnlyList? deletes = null) => + IReadOnlyList? deletes = null, + AcceptProgress? progress = null) => new(true, null, items, window, windowKey) { Moves = moves ?? [], - Deletes = deletes ?? [] + Deletes = deletes ?? [], + Progress = progress }; public string Build() @@ -96,6 +105,12 @@ public string Build() ViewerPayload.Append(builder, "windowKey", WindowKey); } + if (Progress is not null) + { + // Plain too: two counts + builder.Append($"progress: {Progress.Build()}\n"); + } + foreach (var item in Items) { var status = item.Status is null ? "" : ViewerPayload.Encode(item.Status); @@ -143,6 +158,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? string? message = null; WindowCommand? window = null; string? windowKey = null; + AcceptProgress? progress = null; var items = new List(); var moves = new List(); var deletes = new List(); @@ -168,6 +184,13 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? return false; } + continue; + case "progress": + if (!AcceptProgress.TryParse(value, out progress)) + { + return false; + } + continue; case "message": if (!ViewerPayload.TryDecode(value, out message)) @@ -248,7 +271,8 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? response = new(ok.Value, message, items, window, windowKey) { Moves = moves, - Deletes = deletes + Deletes = deletes, + Progress = progress }; return true; } diff --git a/src/DiffEngineTray.Tests/HeldApply.cs b/src/DiffEngineTray.Tests/HeldApply.cs new file mode 100644 index 00000000..6dcc8764 --- /dev/null +++ b/src/DiffEngineTray.Tests/HeldApply.cs @@ -0,0 +1,44 @@ +/// +/// An applier that holds one apply until the test lets it go, so an accept-all can be looked at +/// part way through: what a listing taken then says, and what a viewer displaying the queue shows. +/// +/// Which apply to hold, counting from one. +sealed class HeldApply(int hold) : IDisposable +{ + readonly ManualResetEventSlim reached = new(); + readonly ManualResetEventSlim released = new(); + int count; + + public InlineApplyResult Apply(InlinePatch patch) + { + if (Interlocked.Increment(ref count) == hold) + { + reached.Set(); + released.Wait(TimeSpan.FromSeconds(30)); + } + + return InlineApplyResult.Applied; + } + + /// + /// Blocks until the held apply has started, which is the batch at the point the test is about. + /// + public void WaitUntilHeld() + { + if (!reached.Wait(TimeSpan.FromSeconds(30))) + { + throw new("The held apply was never reached."); + } + } + + public void Release() => + released.Set(); + + /// + /// Never leaves a batch blocked behind a test that failed before letting it go. The events are + /// left for the collector rather than disposed, since the apply being released may not have + /// woken yet. + /// + public void Dispose() => + released.Set(); +} diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index f3045205..66c3af1f 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -21,9 +21,9 @@ public Owner(Func? applier = null) public FakeLauncher Launcher { get; } = new(); public List Warnings { get; } = []; - public ViewerResponse Send(ViewerMessage message) + public ViewerResponse Send(ViewerMessage message, TimeSpan? wait = null) { - if (!ViewerClient.TrySend(message, out var response, Host.Port)) + if (!ViewerClient.TrySend(message, out var response, Host.Port, wait)) { throw new($"No response for {message.Verb}."); } @@ -617,7 +617,16 @@ public bool Has(string key) => return (true, "Discarded tracked"); } - public (int accepted, int kept) AcceptAll() => SweepResult; + public (int accepted, int kept) AcceptAll(Action? advanced = null) + { + // One step per file the sweep reports, which is what the real tracker calls it for + for (var file = 0; file < SweepResult.accepted + SweepResult.kept; file++) + { + advanced?.Invoke(); + } + + return SweepResult; + } public int DiscardAll() => 0; @@ -730,4 +739,78 @@ public async Task AnAcceptAllSweepsTrackedFiles() await Assert.That(response.Message).IsEqualTo("Accepted 1, plus 2 files (1 kept)"); } + + /// + /// An accept-all is one exchange that answers once the whole queue is done, so a viewer + /// displaying the queue learns how it is going from the listings it takes meanwhile: each + /// snapshot leaves as it lands, and the listing says how far the batch has got. + /// + [Test] + public async Task AListingDuringAnAcceptAllSaysHowFarItHasGot() + { + using var held = new HeldApply(2); + using var owner = new Owner(held.Apply); + owner.Queue(@"c:\repo\OtherTests.cs", 7); + owner.Queue(); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + + var partway = owner.Send(new(ViewerVerb.ListFull)); + await Assert.That(partway.Progress).IsEqualTo(new AcceptProgress(1, 2)); + await Assert.That(partway.Items).HasSingleItem(); + + held.Release(); + var response = await accepting; + + await Assert.That(response.Message).IsEqualTo("Accepted 2"); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsNull(); + } + + /// + /// The tracked files are swept first, and a move being retried while a diff tool lets go of it + /// is as much of the wait as any snapshot, so they count towards the progress too. + /// + [Test] + public async Task AnAcceptAllCountsTheFilesIntoItsProgress() + { + using var held = new HeldApply(1); + using var owner = new Owner(held.Apply); + owner.Host.TrackedFiles = new FakeTracked + { + MoveList = [new(@"move:c:\temp\a.txt", "a (txt)", null, @"c:\temp\a.txt", @"c:\code\a.txt")], + DeleteList = [new(@"delete:c:\code\b.txt", "b.txt", null, @"c:\code\b.txt")], + SweepResult = (2, 0) + }; + owner.Queue(); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsEqualTo(new AcceptProgress(2, 3)); + + held.Release(); + await Assert.That((await accepting).Message).IsEqualTo("Accepted 1, plus 2 files"); + } + + /// + /// The menu's accept-all reaches the queue through the tray's own host rather than the wire, + /// and a viewer displaying the queue follows that one the same way. + /// + [Test] + public async Task TheMenusAcceptAllReportsProgressToo() + { + using var held = new HeldApply(1); + using var owner = new Owner(held.Apply); + owner.Queue(); + + var accepting = Task.Run(() => owner.Host.AcceptAll(out _)); + held.WaitUntilHeld(); + + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsEqualTo(new AcceptProgress(0, 1)); + + held.Release(); + await Assert.That(await accepting).IsTrue(); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsNull(); + } } diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index cfb1a51c..4d947374 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -188,6 +188,45 @@ public async Task ViewerAcceptAllEmptiesTheTray() await Assert.That(File.Exists(delete.File)).IsFalse(); } + /// + /// The arrangement the tray sets up at login: it owns the queue and a viewer displays it. An + /// accept-all clicked in that viewer runs in the tray, and the window follows it there - each + /// entry leaving as it lands, and the tray's count in the status line - rather than saying + /// "Waiting for the queue owner." over an unmoving list until the whole batch is done. + /// + /// Through the viewer's own polling loop rather than , since + /// listing beside a forwarded command that is still running is the loop's job. + /// + /// + [Test] + public async Task AViewerAttachedToTheTrayFollowsItsAcceptAll() + { + using var held = new HeldApply(2); + await using var pair = new TrayOwned(held.Apply); + pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + using var cancel = new CancelSource(); + var polling = Task.Run(() => pair.Link.Run(cancel.Token), Cancel.None); + + pair.Link.Post(ViewerSideVerb.AcceptAll, null); + held.WaitUntilHeld(); + // A listing taken while the first entry was still applying can land first, so wait for one + // taken with the second held + await Until(() => pair.Window.State.OwnerProgress is { Done: 1, Total: 2 }); + + await Assert.That(pair.Window.State.Progress!.Describe()).IsEqualTo("Accepting 2 of 2"); + await Assert.That(pair.Window.State.Keys()).IsEquivalentTo([Key(other, 7)]); + + held.Release(); + // The last entry going closes a window that only displays the tray's queue + await Until(() => pair.Window.State.Exit); + await Assert.That(pair.Window.State.OwnerProgress).IsNull(); + await Assert.That(pair.Listing).IsEmpty(); + await cancel.CancelAsync(); + await polling.WaitAsync(TimeSpan.FromSeconds(10)); + } + [Test] public async Task ViewerAcceptOfOneSnapshotReachesTheTray() { @@ -1079,6 +1118,24 @@ public async ValueTask DisposeAsync() static string TempRoot() => Path.Combine(Path.GetTempPath(), $"TrayViewerSync_{Guid.NewGuid():N}"); + + /// + /// For the tests that run the viewer's polling loop for real, which catches up with the tray + /// on its own schedule. + /// + static async Task Until(Func condition) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(30); + while (!condition()) + { + if (DateTime.UtcNow > deadline) + { + throw new("Timed out waiting for the viewer to catch up."); + } + + await Task.Delay(20); + } + } } static class TrayViewerSyncExtensions diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index 616496f9..a296d852 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -29,7 +29,11 @@ interface ITrackedFiles /// Accept every tracked delete and move without prompting. Kept is what stayed pending — /// locked moves, undeletable files. /// - (int accepted, int kept) AcceptAll(); + /// + /// Called as each file is dealt with, whichever way it went, so the owner can say how far an + /// accept-all has got while a locked move is still being retried. + /// + (int accepted, int kept) AcceptAll(Action? advanced = null); /// /// Track a pending move or delete that arrived over the viewer port rather than the piper one. diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index 4c9841da..5a173739 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -35,6 +35,19 @@ sealed class OwnedInlineHost : WindowCommand? window; string? windowKey; + /// + /// How far the running accept-all has got, null when none is. Under , since + /// the listener threads read it into every listing. + /// + AcceptProgress? progress; + + /// + /// One accept-all at a time. The menu, a hot key and a displaying viewer can each ask for one, + /// and two sweeping the same queue at once would apply every snapshot twice and report two + /// sets of progress over each other. A second waits, and then sweeps whatever arrived meanwhile. + /// + readonly Lock accepting = new(); + OwnedInlineHost( ViewerServer server, Action failed, @@ -134,7 +147,19 @@ public bool Discard(PendingSnapshot snapshot, out string? message) public bool AcceptAll(out string? message) { - message = AcceptEvery(); + lock (accepting) + { + StartProgress(0); + try + { + message = AcceptEvery(); + } + finally + { + EndProgress(); + } + } + lock (gate) { // A snapshot that arrived while the batch was applying is still pending, and saying @@ -246,13 +271,13 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) ViewerResponse response; if (withPatches) { - response = ViewerResponse.Listing(items, window, windowKey, moves, deletes); + response = ViewerResponse.Listing(items, window, windowKey, moves, deletes, progress); window = null; windowKey = null; } else { - response = ViewerResponse.Listing(items, null, null, moves, deletes); + response = ViewerResponse.Listing(items, null, null, moves, deletes, progress); } return response; @@ -335,11 +360,30 @@ bool IQueueOwner.Has(string key) /// moves as well as the snapshots, mirroring the tray menu's own "Accept all". Files first, /// the order that menu has always used, and never through , /// whose snapshot half would re-enter this host and whose move path can prompt. + /// + /// The files count towards the progress a listing reports, since a move that is being retried + /// while a diff tool lets go of it is as much of the wait as any snapshot. + /// /// string IQueueOwner.AcceptAll() { - var tracked = TrackedFiles?.AcceptAll(); - var message = AcceptEvery(); + (int accepted, int kept)? tracked; + string message; + lock (accepting) + { + var files = TrackedFiles is { } trackedFiles ? trackedFiles.Moves().Count + trackedFiles.Deletes().Count : 0; + StartProgress(files); + try + { + tracked = TrackedFiles?.AcceptAll(Advance); + message = AcceptEvery(); + } + finally + { + EndProgress(); + } + } + Changed?.Invoke(); if (tracked is not { } swept || swept is { accepted: 0, kept: 0 }) @@ -464,25 +508,84 @@ void IQueueOwner.Window(WindowCommand command, string? key) return (outcome, message, false); } + /// + /// Every snapshot pending when it starts, applied outside the gate and completed one at a + /// time. The list is immutable, so applying over it is safe, and each completion skips an + /// entry that changed underneath it. Conflicted entries are never applied: they are counted + /// into the message at the end. + /// + /// Completed as each lands rather than all together at the end, so a displaying viewer's next + /// listing shows the queue shrinking and says how far the batch has got. Together they left + /// the window showing an untouched queue for as long as the batch took. + /// + /// string AcceptEvery() { - IReadOnlyList pending; + List pending; + lock (gate) + { + pending = queue.Items + .Where(_ => !_.Conflicted) + .ToList(); + // Exact now, where the start could only estimate it: the files swept first gave + // snapshots time to arrive or settle + if (progress is not null) + { + progress = progress with { Total = progress.Done + pending.Count }; + } + } + + var tally = new AcceptAllTally(); + foreach (var entry in pending) + { + var result = applier(entry.Patch); + // Together, so no listing can show the entry gone and the count not yet moved past it + lock (gate) + { + queue = queue.AcceptInBatch(entry, result, ref tally); + progress = progress?.Advance(); + } + + Changed?.Invoke(); + } + + lock (gate) + { + return tally.Message(queue.Conflicts); + } + } + + /// + /// A bulk accept is starting, over tracked files and whatever + /// snapshots can be applied. + /// + void StartProgress(int files) + { lock (gate) { - pending = queue.Items; + progress = new(0, files + queue.Items.Count(_ => !_.Conflicted)); } + } - // The list is immutable, so applying over it outside the gate is safe; the completion - // skips anything that changed underneath. Conflicted entries are never applied: the - // batch completion counts what it skipped into the message. - var outcomes = pending - .Where(_ => !_.Conflicted) - .Select(_ => (_, applier(_.Patch))) - .ToList(); + /// + /// One more tracked file dealt with. Raises , so the tray's own listing + /// follows the batch as a displaying viewer's does. + /// + void Advance() + { lock (gate) { - queue = queue.AcceptAll(outcomes, out var message); - return message; + progress = progress?.Advance(); + } + + Changed?.Invoke(); + } + + void EndProgress() + { + lock (gate) + { + progress = null; } } diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 288bd817..035bf618 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -38,6 +38,14 @@ class RemoteInlineHost : IInlineHost /// static readonly TimeSpan acceptWait = TimeSpan.FromSeconds(15); + /// + /// An accept-all is one exchange holding an apply per entry, so it outlasts a lone accept by as + /// many entries as the queue holds, and fifteen seconds read a long queue as a missing viewer + /// partway through accepting it. The owning viewer says how far it has got in its own window; + /// this only bounds one that took the verb and wedged. On a worker, as every accept here is. + /// + static readonly TimeSpan acceptAllWait = TimeSpan.FromMinutes(5); + public string Description => $"owned by another process on port {ViewerClient.Port}"; public IReadOnlyList List() => @@ -117,10 +125,8 @@ public bool Discard(PendingSnapshot snapshot, out string? message) => /// and matching what an owning tray reports, which is also "is anything still pending". A /// conflict counts as not accepted, which is right: it is what a reviewer still has to resolve. /// - // One accept per entry inside a single exchange, so this outlasts a lone accept rather than - // matching it public bool AcceptAll(out string? message) => - Send(ViewerVerb.AcceptAll, null, acceptWait, out message) && + Send(ViewerVerb.AcceptAll, null, acceptAllWait, out message) && List().Count == 0; /// diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 94285b82..7b20899f 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -866,7 +866,7 @@ bool ITrackedFiles.Untrack(string key) return (false, null); } - (int accepted, int kept) ITrackedFiles.AcceptAll() + (int accepted, int kept) ITrackedFiles.AcceptAll(Action? advanced) { var accepted = 0; var kept = 0; @@ -880,6 +880,8 @@ bool ITrackedFiles.Untrack(string key) { kept++; } + + advanced?.Invoke(); } foreach (var move in moves.Values.ToList()) @@ -892,6 +894,8 @@ bool ITrackedFiles.Untrack(string key) { kept++; } + + advanced?.Invoke(); } return (accepted, kept); diff --git a/src/DiffEngineViewer.Tests/AcceptAllProgressTests.cs b/src/DiffEngineViewer.Tests/AcceptAllProgressTests.cs new file mode 100644 index 00000000..9d6837ad --- /dev/null +++ b/src/DiffEngineViewer.Tests/AcceptAllProgressTests.cs @@ -0,0 +1,392 @@ +/// +/// An accept-all takes as long as the queue is long, so it goes an entry at a time: each one +/// leaves the list as it lands, the status line says how far the batch has got, and the window +/// keeps drawing throughout - whichever process is holding the queue. +/// +[NotInParallel] +public class AcceptAllProgressTests +{ + [Test] + public async Task EachEntryLeavesAsItLands() + { + var state = ViewerSession.BeginAcceptAll(Pending()); + var seen = new List(); + while ((state = ViewerSession.ClaimNext(state)).Batch?.Current is { } entry) + { + seen.Add($"{ScreenBuilder.Build(state).Status}, {state.Queue.Count} pending"); + state = ViewerSession.ApplyClaimed(entry, Fixtures.Applied)(state); + } + + await Assert.That(string.Join("\n", seen)).IsEqualTo( + """ + Accepting 1 of 3, 3 pending + Accepting 2 of 3, 2 pending + Accepting 3 of 3, 1 pending + """); + await Assert.That(state.Progress).IsNull(); + await Assert.That(state.Message).IsEqualTo("Accepted 3"); + await Assert.That(state.Exit).IsTrue(); + } + + /// + /// The files are part of the batch as much as the snapshots are, and come after them: whether + /// a delete is held turns on how the snapshots went. + /// + [Test] + public async Task TheFilesAreCountedAfterTheSnapshots() + { + var done = new List(); + var actions = Fixtures.Applied with + { + MoveFile = (temp, _) => done.Add(temp), + DeleteFile = done.Add + }; + var state = ViewerSession.EnqueueTracked(Pending(), Fixtures.Move()); + state = ViewerSession.EnqueueTracked(state, Fixtures.Delete()); + + state = ViewerSession.BeginAcceptAll(state); + var kinds = new List(); + while ((state = ViewerSession.ClaimNext(state)).Batch?.Current is { } entry) + { + kinds.Add($"{state.Progress!.Describe()}: {entry.Kind}"); + state = ViewerSession.ApplyClaimed(entry, actions)(state); + } + + await Assert.That(string.Join("\n", kinds)).IsEqualTo( + """ + Accepting 1 of 5: Inline + Accepting 2 of 5: Inline + Accepting 3 of 5: Inline + Accepting 4 of 5: Move + Accepting 5 of 5: Delete + """); + await Assert.That(state.Message).IsEqualTo("Accepted 3, plus 2 files"); + await Assert.That(done.Count).IsEqualTo(2); + } + + /// + /// A test that starts passing while the batch is under way settles its entry, and the batch + /// passes over it rather than accepting a snapshot that is no longer pending. + /// + [Test] + public async Task AnEntrySettledPartWayIsPassedOver() + { + var applied = new List(); + var actions = Fixtures.Applied with + { + ApplyInline = _ => + { + applied.Add(_); + return InlineApplyResult.Applied; + } + }; + var state = ViewerSession.BeginAcceptAll(Pending()); + state = ViewerSession.ClaimNext(state); + var first = state.Batch!.Current!; + state = ViewerSession.ApplyClaimed(first, actions)(state); + + var next = state.Batch!.Remaining[0]; + state = ViewerSession.Settle(state, next); + while ((state = ViewerSession.ClaimNext(state)).Batch?.Current is { } entry) + { + state = ViewerSession.ApplyClaimed(entry, actions)(state); + } + + await Assert.That(applied.Count).IsEqualTo(2); + await Assert.That(state.Message).IsEqualTo("Accepted 2"); + } + + /// + /// A still failing test that re-runs while its snapshot is being written sends different + /// content, and that content is the news: the outcome describes what was there before, so the + /// entry stays with the new content and the batch does not count it. + /// + [Test] + public async Task AReRunWhileItAppliesKeepsTheNewContent() + { + var state = ViewerSession.BeginAcceptAll(Fixtures.Inline(Fixtures.Patch())); + state = ViewerSession.ClaimNext(state); + var record = ViewerSession.ApplyClaimed(state.Batch!.Current!, Fixtures.Applied); + + state = ViewerSession.EnqueueInline(state, Fixtures.Patch(content: "third run")); + state = ViewerSession.ClaimNext(record(state)); + + await Assert.That(state.Queue.Single().LeftText).IsEqualTo("third run"); + await Assert.That(state.Batch).IsNull(); + await Assert.That(state.Message).IsEqualTo("Accepted 0"); + } + + /// + /// A second framework reporting different content for an entry the batch has not reached yet + /// makes a conflict of it, and a bulk accept never picks a side. + /// + [Test] + public async Task AnEntryThatBecameAConflictIsLeftForReview() + { + var state = ViewerSession.BeginAcceptAll( + Fixtures.Inline( + Fixtures.Patch("ATests.cs", 1, framework: "net8.0"), + Fixtures.Patch("BTests.cs", 2, framework: "net8.0"))); + state = ViewerSession.ClaimNext(state); + state = ViewerSession.ApplyClaimed(state.Batch!.Current!, Fixtures.Applied)(state); + + state = ViewerSession.EnqueueInline(state, Fixtures.Patch("BTests.cs", 2, content: "nine", framework: "net9.0")); + state = ViewerSession.ClaimNext(state); + + await Assert.That(state.Batch).IsNull(); + await Assert.That(state.Queue.Single().Conflicted).IsTrue(); + await Assert.That(state.Message).IsEqualTo("Accepted 1, 1 conflict needs review"); + } + + /// + /// Nothing to apply is nothing to wait for: no progress to report, and the message at once. + /// + [Test] + public async Task AQueueOfConflictsFinishesWhereItStarts() + { + var state = ViewerSession.BeginAcceptAll( + Fixtures.Inline( + Fixtures.Patch(content: "eight", framework: "net8.0"), + Fixtures.Patch(content: "nine", framework: "net9.0"))); + + await Assert.That(state.Batch).IsNull(); + await Assert.That(state.Message).IsEqualTo("Accepted 0, 1 conflict needs review"); + } + + /// + /// A batch that is running is the one a second request was asking for, so starting another + /// changes nothing. + /// + [Test] + public async Task ASecondBeginLeavesTheRunningBatch() + { + var running = ViewerSession.ClaimNext(ViewerSession.BeginAcceptAll(Pending())); + + await Assert.That(ViewerSession.BeginAcceptAll(running)).IsSameReferenceAs(running); + } + + /// + /// The window refuses what would change the queue under the batch, so the buttons that would + /// are disabled, and the keys and a second accept-all do nothing. Looking around carries on. + /// + [Test] + public async Task TheWindowOffersNothingThatChangesTheQueueWhileItRuns() + { + var state = ViewerSession.ClaimNext(ViewerSession.BeginAcceptAll(Pending())); + var screen = ScreenBuilder.Build(state); + + await Assert.That(screen.Status).IsEqualTo("Accepting 1 of 3"); + await Assert.That(screen.Buttons.Where(_ => _.Enabled)).IsEmpty(); + + var window = new Window(); + foreach (var key in new[] { CommandKind.Accept, CommandKind.Discard, CommandKind.AcceptAll }) + { + var pressed = ViewerProgram.Apply(state, Input(key), null, window); + await Assert.That(pressed.Queue).IsSameReferenceAs(state.Queue); + await Assert.That(pressed.Batch).IsEqualTo(state.Batch); + } + + var stepped = ViewerProgram.Apply(state, Input(CommandKind.NextItem), null, window); + await Assert.That(stepped.Selected).IsEqualTo(state.Selected + 1); + } + + /// + /// A viewer displaying someone else's queue shows the owner's batch the same way, from what + /// the owner's listing said about it. + /// + [Test] + public async Task AnAttachedWindowShowsTheOwnersProgress() + { + var state = ViewerSession.Sync( + Fixtures.Attached(Fixtures.Pending(Fixtures.Patch())), + Fixtures.Pending(Fixtures.Patch()), + [], + null, + new(4, 9)); + + var screen = ScreenBuilder.Build(state); + await Assert.That(screen.Status).IsEqualTo("Accepting 5 of 9"); + await Assert.That(screen.Buttons.Where(_ => _.Enabled)).IsEmpty(); + + // And the listing after the batch is what gives the window back + var after = ViewerSession.Sync(state, Fixtures.Pending(Fixtures.Patch()), [], "Accepted 8", null); + await Assert.That(ScreenBuilder.Build(after).Status).IsEqualTo("Accepted 8"); + } + + /// + /// The point of the runner: the lock is free while an entry applies, so the render loop - which + /// takes it every frame - keeps drawing, and what it draws is the batch as it now stands. + /// + [Test] + public async Task TheRunnerAppliesOutsideTheLock() + { + var host = new SessionHost(Pending()); + var seen = new List(); + var actions = Fixtures.Applied with + { + ApplyInline = _ => + { + // Another thread, standing in for the render loop, since the lock lets the + // thread that holds it back in + var drawn = Task.Run(() => host.Mutate(state => state)); + if (!drawn.Wait(TimeSpan.FromSeconds(10))) + { + throw new("The lock was held across an apply."); + } + + seen.Add(ScreenBuilder.Build(drawn.Result).Status); + return InlineApplyResult.Applied; + } + }; + host.Mutate(ViewerSession.BeginAcceptAll); + + var message = new AcceptAllRunner(host, actions).Drive(); + + await Assert.That(string.Join("\n", seen)).IsEqualTo( + """ + Accepting 1 of 3 + Accepting 2 of 3 + Accepting 3 of 3 + """); + await Assert.That(message).IsEqualTo("Accepted 3"); + await Assert.That(host.State.Queue).IsEmpty(); + } + + /// + /// An applier that throws rather than answering still leaves a batch that finishes, with the + /// throw recorded against the entry like any other failure. + /// + [Test] + public async Task AThrowingApplyStillFinishesTheBatch() + { + var host = new SessionHost(Pending()); + var actions = Fixtures.Applied with + { + ApplyInline = _ => _.LineHint == 88 ? throw new("the disk went away") : InlineApplyResult.Applied + }; + host.Mutate(ViewerSession.BeginAcceptAll); + + var message = new AcceptAllRunner(host, actions).Drive(); + + await Assert.That(message).IsEqualTo("Accepted 2, 1 failed. the disk went away"); + await Assert.That(host.State.Batch).IsNull(); + await Assert.That(host.State.Queue.Single().Status).IsEqualTo("the disk went away"); + } + + /// + /// The tray's accept-all against a viewer that owns the queue: the listener thread carries it + /// out, and a listing taken part way through says how far it has got and no longer lists what + /// has been accepted - which is the window's view of it too. + /// + [Test] + public async Task AListingDuringAWireAcceptAllSaysHowFarItHasGot() + { + using var held = new HeldApply(2); + using var owner = new ServerFixture(applier: held.Apply); + owner.Send(Inline(Fixtures.Patch())); + owner.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); + + // Waited on for as long as the test holds the batch, which the client's default does not + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + + var partway = owner.Send(new(ViewerVerb.ListFull)); + await Assert.That(partway.Progress).IsEqualTo(new AcceptProgress(1, 2)); + await Assert.That(partway.Items).HasSingleItem(); + await Assert.That(ScreenBuilder.Build(owner.Host.State).Status).IsEqualTo("Accepting 2 of 2"); + + held.Release(); + var response = await accepting; + + await Assert.That(response.Message).IsEqualTo("Accepted 2"); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsNull(); + } + + /// + /// A viewer displaying the queue follows an accept-all it forwarded while the owner is still + /// carrying it out, rather than waiting on the one exchange that answers only at the end. + /// + [Test] + public async Task AnAttachedViewerFollowsTheOwnerThroughTheBatch() + { + using var held = new HeldApply(2); + using var owner = new ServerFixture(applier: held.Apply); + owner.Send(Inline(Fixtures.Patch())); + owner.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); + var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows)); + var link = new OwnerLink(host, owner.Server.Port); + using var cancel = new CancelSource(); + var polling = Task.Run(() => link.Run(cancel.Token), Cancel.None); + await Until(() => host.State.Queue.Count == 2); + + link.Post(ViewerVerb.AcceptAll, null); + held.WaitUntilHeld(); + // A listing taken while the first entry was still applying can land first, so wait for one + // taken with the second held + await Until(() => host.State.OwnerProgress is { Done: 1, Total: 2 }); + + await Assert.That(ScreenBuilder.Build(host.State).Status).IsEqualTo("Accepting 2 of 2"); + await Assert.That(host.State.Queue).HasSingleItem(); + + held.Release(); + // The last entry going closes a window that is only displaying someone else's queue + await Until(() => host.State.Exit); + await Assert.That(host.State.OwnerProgress).IsNull(); + await cancel.CancelAsync(); + await polling.WaitAsync(TimeSpan.FromSeconds(10)); + } + + static async Task Until(Func condition) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(30); + while (!condition()) + { + if (DateTime.UtcNow > deadline) + { + throw new("Timed out waiting for the window to catch up."); + } + + await Task.Delay(20); + } + } + + static ViewerMessage Inline(InlinePatch patch) => + new(ViewerVerb.Inline, Body: InlinePatchFile.Build(patch)); + + static SessionState Pending() => + Fixtures.Inline( + Fixtures.Patch(), + Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "two"), + Fixtures.Patch("OtherTests.cs", 12, null, "brand new")); + + static ViewerInput Input(CommandKind key) => + new(key, -1, -1, 0, false, Fixtures.Columns, Fixtures.Rows); + + sealed class Window : IViewerWindow + { + public bool Present(Screen screen) => + true; + + public ViewerInput Poll() => + default; + + public void SetHidden(bool hidden) + { + } + + public void Focus() + { + } + + public void SetClipboard(string text) + { + } + + public bool Capture(Screen screen, int width, int height, string pngPath) => + false; + + public void Dispose() + { + } + } +} diff --git a/src/DiffEngineViewer.Tests/HeldApply.cs b/src/DiffEngineViewer.Tests/HeldApply.cs new file mode 100644 index 00000000..99666ea8 --- /dev/null +++ b/src/DiffEngineViewer.Tests/HeldApply.cs @@ -0,0 +1,44 @@ +/// +/// An applier that holds one apply until the test lets it go, so an accept-all can be looked at +/// part way through: what the window shows, and what a listing taken then says. +/// +/// Which apply to hold, counting from one. +sealed class HeldApply(int hold) : IDisposable +{ + readonly ManualResetEventSlim reached = new(); + readonly ManualResetEventSlim released = new(); + int count; + + public InlineApplyResult Apply(InlinePatch patch) + { + if (Interlocked.Increment(ref count) == hold) + { + reached.Set(); + released.Wait(TimeSpan.FromSeconds(30)); + } + + return InlineApplyResult.Applied; + } + + /// + /// Blocks until the held apply has started, which is the batch at the point the test is about. + /// + public void WaitUntilHeld() + { + if (!reached.Wait(TimeSpan.FromSeconds(30))) + { + throw new("The held apply was never reached."); + } + } + + public void Release() => + released.Set(); + + /// + /// Never leaves a batch blocked behind a test that failed before letting it go. The events are + /// left for the collector rather than disposed, since the apply being released may not have + /// woken yet. + /// + public void Dispose() => + released.Set(); +} diff --git a/src/DiffEngineViewer.Tests/InlineScreenTests.AcceptAllInProgress.verified.txt b/src/DiffEngineViewer.Tests/InlineScreenTests.AcceptAllInProgress.verified.txt new file mode 100644 index 00000000..93d6f68a --- /dev/null +++ b/src/DiffEngineViewer.Tests/InlineScreenTests.AcceptAllInProgress.verified.txt @@ -0,0 +1,24 @@ ++----------------------------------------------------------------------------------------------+ +| OtherTests.cs:12 inline 1 of 3 | ++----------------------+-----------------------------------+-----------------------------------+ +| Pending (3) | received | expected (new snapshot) | ++----------------------+-----------------------------------+-----------------------------------+ +| > OtherTests.cs:12 | + 1 brand new | | +| OtherTests.cs:30 | | | +| WideNamedTests.cs> | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | +| | | | ++----------------------+-----------------------------------+-----------------------------------+ +| (Accept) (Discard) (Accept all) Accepting 3 of 5 | ++----------------------------------------------------------------------------------------------+ \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/InlineScreenTests.cs b/src/DiffEngineViewer.Tests/InlineScreenTests.cs index f7d563fc..12783a76 100644 --- a/src/DiffEngineViewer.Tests/InlineScreenTests.cs +++ b/src/DiffEngineViewer.Tests/InlineScreenTests.cs @@ -36,6 +36,25 @@ public Task AfterDiscard() => public Task AfterAcceptAll() => Verify(Fixtures.Render(ViewerSession.Apply(Pending(), CommandKind.AcceptAll, Fixtures.Applied))); + /// + /// Part way through an accept-all: what has been accepted has left the list, the status line + /// says how far the batch has got, and nothing that changes the queue is offered until it is + /// done. + /// + [Test] + public Task AcceptAllInProgress() + { + var state = ViewerSession.BeginAcceptAll(Pending()); + for (var step = 0; step < 2; step++) + { + state = ViewerSession.ClaimNext(state); + state = ViewerSession.ApplyClaimed(state.Batch!.Current!, Fixtures.Applied)(state); + } + + // The third claimed, and still applying + return Verify(Fixtures.Render(ViewerSession.ClaimNext(state))); + } + [Test] public Task StalePatch() { diff --git a/src/DiffEngineViewer.Tests/ServerFixture.cs b/src/DiffEngineViewer.Tests/ServerFixture.cs index d915f4d7..7aa99792 100644 --- a/src/DiffEngineViewer.Tests/ServerFixture.cs +++ b/src/DiffEngineViewer.Tests/ServerFixture.cs @@ -8,7 +8,11 @@ sealed class ServerFixture : IDisposable readonly CancelSource cancel = new(); readonly Task listening; - public ServerFixture(ViewerMode mode = ViewerMode.Inline) + /// + /// What applying answers, and when, for a test that needs a batch held part way through. + /// Applied, at once, otherwise. + /// + public ServerFixture(ViewerMode mode = ViewerMode.Inline, Func? applier = null) { Host = new(SessionState.Start(mode, Fixtures.Columns, Fixtures.Rows)); if (!ViewerServer.TryBind(0, out var server)) @@ -20,8 +24,12 @@ public ServerFixture(ViewerMode mode = ViewerMode.Inline) var actions = new ViewerActions( patch => { - Applied.Add(patch); - return InlineApplyResult.Applied; + lock (Applied) + { + Applied.Add(patch); + } + + return applier?.Invoke(patch) ?? InlineApplyResult.Applied; }, (_, _) => { }, _ => { }); @@ -34,9 +42,9 @@ public ServerFixture(ViewerMode mode = ViewerMode.Inline) public List Applied { get; } = []; public List Windows { get; } = []; - public ViewerResponse Send(ViewerMessage message) + public ViewerResponse Send(ViewerMessage message, TimeSpan? wait = null) { - if (!ViewerClient.TrySend(message, out var response, Server.Port)) + if (!ViewerClient.TrySend(message, out var response, Server.Port, wait)) { throw new($"No response for {message.Verb}."); } diff --git a/src/DiffEngineViewer.Tests/ViewerLaunchTests.cs b/src/DiffEngineViewer.Tests/ViewerLaunchTests.cs index d20cd7e0..2ea7189a 100644 --- a/src/DiffEngineViewer.Tests/ViewerLaunchTests.cs +++ b/src/DiffEngineViewer.Tests/ViewerLaunchTests.cs @@ -357,6 +357,255 @@ public async Task GroupedQueue() } } + /// + /// Enough pending snapshots that accepting them takes a while, for watching an accept-all go: + /// the status line counting up, entries leaving as they land, and the window answering the whole + /// time. Two solutions of ten classes, twenty five snapshots a class, which is the shape a run + /// that changed something widely used leaves behind. Every accept rewrites a file holding two + /// dozen others, and the multi line literal it writes moves every call site below it, so the + /// rest of the class is found by what it says rather than by the line it was reported on. + /// + /// A fast disk accepts all of that in a second or two, which is not long enough to watch or to + /// try anything while it runs. So the class halfway down the batch is held the way another + /// process part way through writing it would hold it - an IDE accepting its own snapshot - and + /// the batch stops there for a few seconds. That is also the case the window used to freeze in. + /// + /// + /// Three conflicts ride along. Accept-all leaves those for review, so the window is still open + /// once the batch has finished, and what it says about the batch can be read. + /// + /// + /// On its own port, for the reason gives. + /// + /// + [Test] + [Explicit] + public async Task AcceptAllOverALongQueue() + { + const int classes = 10; + const int snapshots = 25; + const int conflicts = 3; + Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", "3499"); + try + { + var root = ManualViewer.TempDirectory(); + var solutionA = root.CreateSubdirectory("SolutionA"); + await File.WriteAllTextAsync(Path.Combine(solutionA.FullName, "SolutionA.slnx"), ""); + var solutionB = root.CreateSubdirectory("SolutionB"); + await File.WriteAllTextAsync(Path.Combine(solutionB.FullName, "SolutionB.slnx"), ""); + + var sources = new List(); + var patches = new List(); + foreach (var solution in new[] { solutionA, solutionB }) + { + for (var index = 1; index <= classes; index++) + { + var (source, lines) = WriteClass(solution, $"{solution.Name[^1]}{index:D2}Tests.cs", snapshots); + sources.Add(source); + for (var method = 1; method <= snapshots; method++) + { + patches.Add( + new(source, lines[method - 1], $"\"old {method}\"", Snapshot(method)) + { + TestName = null, + MemberName = $"Case{method}" + }); + } + } + } + + var (conflicted, conflictedLines) = WriteClass(solutionB, "ConflictedTests.cs", conflicts); + for (var method = 1; method <= conflicts; method++) + { + var line = conflictedLines[method - 1]; + patches.Add(Patch(conflicted, line, $"\"old {method}\"", $"eight {method}", framework: "net8.0")); + patches.Add(Patch(conflicted, line, $"\"old {method}\"", $"nine {method}", framework: "net9.0")); + } + + var total = sources.Count * snapshots; + ManualViewer.Expect( + "Accept all over a long queue", + $"Pending ({total + conflicts}), grouped under SolutionA and SolutionB headers - the list follows the last arrival to the bottom, so scroll it up to see them", + "Press Accept all, or Shift+A", + $"The status line counts up - Accepting 1 of {total}, 2 of {total} - and entries leave the list as they land", + "Accept, Discard and Accept all are disabled until it finishes, and a, d and Shift+A do nothing", + "About halfway the count stops for six seconds, on a class this test is holding the way another process writing it would", + "The window keeps answering the whole time, stop included: scroll, Tab through the entries, click one, fold a header", + "The count carries on once the class is let go", + $"When it finishes the {conflicts} ConflictedTests.cs entries are left, and the status line says Accepted {total}, {conflicts} conflicts need review"); + + foreach (var patch in patches) + { + var result = await EngineRunner.AddInlineAsync(patch); + await Assert.That(result).IsEqualTo(EngineResult.Queued); + } + + // The batch goes in the order the queue lists, so the class listed halfway down is the + // one it reaches halfway through + if (!ViewerClient.TrySend(new(ViewerVerb.List), out var listing, 3499)) + { + throw new("The viewer did not list its queue."); + } + + var halfway = listing.Items[listing.Items.Count / 2].Name.Split(':')[0]; + using var cancel = new CancelSource(); + var holding = HoldOnceReached(sources.Single(_ => Path.GetFileName(_) == halfway), 3499, cancel.Token); + + await ManualViewer.WaitForClose(); + await cancel.CancelAsync(); + await holding; + + var left = sources.Sum(_ => Unaccepted(_)); + Console.WriteLine(); + Console.WriteLine($"{total - left} of {total} call sites rewritten, and {Unaccepted(conflicted)} of {conflicts} conflicted ones left alone."); + // One class in full, for what the accepts wrote. The other nineteen are the same shape. + Report(sources[0]); + } + finally + { + Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", null); + } + } + + /// + /// Holds the way another process applying a patch to it would, by + /// taking the cross process mutex InlineApplier waits on, so an accept-all stops when it gets + /// there. + /// + /// Taken only once a batch has started, so someone who accepts one at a time instead is never + /// held up by it. Let go six seconds after the count stops moving, well inside the ten seconds + /// InlineApplier waits before giving up, so the held entry still lands. + /// + /// + /// A thread of its own, because a mutex has to be released by the thread that took it. + /// + /// + static Task HoldOnceReached(string source, int port, Cancel cancel) => + Task.Factory.StartNew( + () => + { + AcceptProgress? Progress() => + ViewerClient.TrySend(new(ViewerVerb.List), out var response, port) ? response.Progress : null; + + while (Progress() is null) + { + if (cancel.WaitHandle.WaitOne(TimeSpan.FromMilliseconds(50))) + { + return; + } + } + + using var mutex = new Mutex(false, PatchMutex(source)); + if (!mutex.WaitOne(TimeSpan.FromSeconds(5))) + { + return; + } + + try + { + // Stopped, rather than between two entries, once the count has stood still for + // longer than an apply takes + var previous = Progress(); + var since = DateTime.UtcNow; + while (previous is not null && + DateTime.UtcNow - since < TimeSpan.FromMilliseconds(300)) + { + if (cancel.WaitHandle.WaitOne(TimeSpan.FromMilliseconds(50))) + { + return; + } + + var current = Progress(); + if (current != previous) + { + previous = current; + since = DateTime.UtcNow; + } + } + + // A batch that finished instead got past this class before it was taken + if (previous is not null) + { + cancel.WaitHandle.WaitOne(TimeSpan.FromSeconds(6)); + } + } + finally + { + mutex.ReleaseMutex(); + } + }, + // Watched inside rather than handed over: a token here cancels the scheduling, and a + // task that never ran would fault the await after the window closes + Cancel.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + + /// + /// The name InlineApplier gives the mutex it waits on for a file. Worked out the same way here + /// rather than shared, because nothing but this test needs it; if the two ever part, the + /// batch simply does not stop, and the rest of the test still stands. + /// + static string PatchMutex(string source) + { + var hash = System.Security.Cryptography.SHA256.HashData( + Encoding.UTF8.GetBytes(Path.GetFullPath(source).ToLowerInvariant())); + return $"DiffEngineInline_{Convert.ToHexString(hash)}"; + } + + /// + /// A class of tests, each verifying and holding a snapshot, in the + /// shape writes one of. Returns the line of each verify call, which is + /// what a patch reports. + /// + static (string Path, IReadOnlyList Lines) WriteClass(DirectoryInfo directory, string name, int methods) + { + var builder = new StringBuilder("public class Sample\n{\n"); + var lines = new List(); + for (var method = 1; method <= methods; method++) + { + // The class opens on two lines, each method takes seven, and the verify call is the + // fourth of them + lines.Add(2 + (method - 1) * 7 + 4); + builder.Append(" [Test]\n"); + builder.Append($" public Task Case{method}()\n"); + builder.Append(" {\n"); + builder.Append(" Verify(Build())\n"); + builder.Append($" .Snapshot(\"old {method}\");\n"); + builder.Append(" }\n"); + builder.Append('\n'); + } + + builder.Append(" static string Build() =>\n"); + builder.Append(" \"content\";\n"); + builder.Append("}\n"); + return (Write(directory, name, builder.ToString()), lines); + } + + /// + /// Several lines, so accepting one writes a raw string literal and moves what follows it. + /// + static string Snapshot(int method) => + $$""" + { + Case: {{method}}, + Value: new + } + """; + + static int Unaccepted(string source) + { + var text = File.ReadAllText(source); + var count = 0; + var index = 0; + while ((index = text.IndexOf(".Snapshot(\"old ", index, StringComparison.Ordinal)) >= 0) + { + count++; + index++; + } + + return count; + } + /// /// Line 6 is the verify call in both shapes, which is what every patch above points at. /// diff --git a/src/DiffEngineViewer/AcceptAllRunner.cs b/src/DiffEngineViewer/AcceptAllRunner.cs new file mode 100644 index 00000000..22f1c967 --- /dev/null +++ b/src/DiffEngineViewer/AcceptAllRunner.cs @@ -0,0 +1,102 @@ +/// +/// Carries out an accept-all over a queue this process owns: claims an entry under the session's +/// lock, applies it outside, records it under the lock again, and repeats until nothing is left. +/// +/// Applying is where the time goes. A snapshot waits on InlineApplier's cross process mutex before +/// it rewrites its source, and a move can fail and be retried, so the lock is never held across +/// one: the render loop takes it every frame, and holding it for a whole batch was the window +/// freezing for a whole batch. Between entries the window draws the queue as it now is, with +/// in its status line. +/// +/// +/// One per owning process, shared by the two places an accept-all starts: the window, which hands +/// it to because its own thread has to keep drawing, and the socket, whose +/// listener thread runs itself because the caller is waiting for the answer. +/// Two drives take turns rather than claiming the same entries, and the second finds nothing left. +/// +/// +sealed class AcceptAllRunner(SessionHost host, ViewerActions actions) +{ + readonly Lock driving = new(); + Task? background; + volatile bool busy; + + /// + /// Whatever batch the session holds, carried out on a worker. From the render loop only, which + /// is what makes safe to read and write without a lock. A no-op while + /// a drive is already under way, whichever thread it is on. + /// + public void Start() + { + if (busy || + background is { IsCompleted: false }) + { + return; + } + + background = Task.Run(Drive, Cancel.None); + } + + /// + /// Runs the session's batch to the end. Returns what the batch said once it finished: the + /// message of the transition that finished it, rather than whatever the state holds by the + /// time the caller reads it, since a settle arriving in between clears the status line. + /// + public string? Drive() + { + lock (driving) + { + busy = true; + try + { + while (true) + { + var claimed = host.Mutate(ViewerSession.ClaimNext); + if (claimed.Batch?.Current is not { } entry) + { + return claimed.Message; + } + + Func record; + try + { + record = ViewerSession.ApplyClaimed(entry, actions); + } + catch (Exception exception) + { + record = ViewerSession.FailClaimed(entry, exception.Message); + } + + host.Mutate(record); + } + } + finally + { + busy = false; + } + } + } + + /// + /// Waits out a batch in flight, for an owner on its way out: what it stages to disk afterwards + /// should be the queue the batch left, not one it was part way through. The window has gone by + /// then, so this is only ever waiting on file operations that are each bounded. + /// + public void Finish() + { + try + { + background?.Wait(); + } + catch (AggregateException) + { + // A drive that threw has nothing more to wait for, and the owner still has a queue to + // stage on its way out + } + + lock (driving) + { + // Taken only to wait for a listener thread's drive to let go + } + } +} diff --git a/src/DiffEngineViewer/AcceptBatch.cs b/src/DiffEngineViewer/AcceptBatch.cs new file mode 100644 index 00000000..72e8b883 --- /dev/null +++ b/src/DiffEngineViewer/AcceptBatch.cs @@ -0,0 +1,43 @@ +/// +/// An accept-all in flight over a queue this process owns. +/// +/// Carried out an entry at a time rather than in one transition. Accepting a snapshot reads, +/// patches and rewrites its source under a cross process mutex, and one transition over the whole +/// queue ran all of that on the render thread: the window stopped painting for as long as the +/// queue was long, and then emptied all at once. Now each entry is claimed under the session's +/// lock, applied outside it, and recorded under it again, so between entries the window draws the +/// queue shrinking and says how far the batch has got. +/// +/// +/// +/// The keys still to do, in the order they will be done: every snapshot before any file, because +/// whether a delete is held turns on how the snapshots went. +/// +/// How many the batch set out with, for the progress it reports. +record AcceptBatch(IReadOnlyList Remaining, int Total) +{ + /// + /// How the snapshots have gone, which is the first half of what the batch says when it is done. + /// + public AcceptAllTally Tally { get; init; } + + /// + /// Moves and deletes carried out. + /// + public int Swept { get; init; } + + /// + /// Moves and deletes still pending afterwards: failed, or held because a snapshot was not + /// written. + /// + public int Kept { get; init; } + + /// + /// The entry claimed and being applied outside the session's lock, as it was when claimed, + /// which is what recording the outcome checks the queue against. Null between entries. + /// + public QueueEntry? Current { get; init; } + + public AcceptProgress Progress => + new(Total - Remaining.Count - (Current is null ? 0 : 1), Total); +} diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index e5025afa..11017b39 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -4,9 +4,19 @@ /// stays here is the projection into , so the display follows the queue /// in the same mutation — acting on an entry selects it, and a focus lands on its item. /// -class MessageHandler(SessionHost host, ViewerActions actions, Action window) : +/// +/// The window's, when there is one, so an accept-all from the tray and one clicked in the window +/// take turns rather than claiming the same entries. A handler with no window makes its own. +/// +class MessageHandler( + SessionHost host, + ViewerActions actions, + Action window, + AcceptAllRunner? runner = null) : IQueueOwner { + readonly AcceptAllRunner runner = runner ?? new(host, actions); + public ViewerResponse Handle(ViewerMessage message) => ViewerMessageHandler.Handle(this, message); @@ -52,7 +62,9 @@ void IQueueOwner.TrackDelete(string file) => /// ViewerResponse IQueueOwner.Listing(bool withPatches) { - var queue = host.State.Queue; + // One read, so the queue and the progress describe the same moment of a batch + var state = host.State; + var queue = state.Queue; var items = ViewerListing.Items( queue .Where(_ => _.Kind == QueueEntryKind.Inline) @@ -60,7 +72,7 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) withPatches); if (!withPatches) { - return ViewerResponse.Listing(items); + return ViewerResponse.Listing(items, progress: state.Progress); } return ViewerResponse.Listing( @@ -72,7 +84,8 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) deletes: queue .Where(_ => _.Kind == QueueEntryKind.Delete) .Select(_ => new ViewerResponseDelete(_.Key, _.Name, _.Solution, _.LeftFile!)) - .ToList()); + .ToList(), + progress: state.Progress); } bool IQueueOwner.Has(string key) => @@ -122,8 +135,17 @@ origin is null && return (true, state.Message); } - string? IQueueOwner.AcceptAll() => - host.Mutate(_ => ViewerSession.Apply(_, CommandKind.AcceptAll, actions)).Message; + /// + /// Started and carried out here, on the listener thread, since the tray asking is waiting for + /// the answer - but a mutation per entry rather than one around the whole batch. Held as one, + /// the lock kept the render loop out for as long as the queue took to apply, so the window + /// froze exactly when there was progress to show. + /// + string? IQueueOwner.AcceptAll() + { + host.Mutate(ViewerSession.BeginAcceptAll); + return runner.Drive(); + } string? IQueueOwner.DiscardAll() => host.Mutate(_ => ViewerSession.Apply(_, CommandKind.DiscardAll, actions)).Message; diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index 695466af..dfab9b72 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -36,12 +36,29 @@ sealed class OwnerLink(SessionHost host, int port) /// public static TimeSpan Wait { get; set; } = TimeSpan.FromSeconds(15); + /// + /// How long a posted command may take, which for an accept-all is as long as the queue is + /// long. The listing taken beside it is what says whether the owner is still there, so this + /// only has to bound one that took the command and wedged; fifteen seconds said a long queue's + /// owner had gone while it was part way through accepting it. + /// + public static TimeSpan SendWait { get; set; } = TimeSpan.FromMinutes(5); + readonly ConcurrentQueue outbound = new(); + /// + /// Set by a post and by a send finishing, so the loop answers either at once rather than on + /// its next interval. + /// + readonly AutoResetEvent wake = new(false); + record Outbound(ViewerVerb Verb, string? Key, string? Body); - public void Post(ViewerVerb verb, string? key, string? body = null) => + public void Post(ViewerVerb verb, string? key, string? body = null) + { outbound.Enqueue(new(verb, key, body)); + wake.Set(); + } public bool Pump() => Pump(out _); @@ -50,8 +67,18 @@ public bool Pump() => /// Send everything posted since the last pass, then read the queue back. Returns false when /// the owner has gone, reported rather than acted on so the caller can decide whether that /// means "do not open a window" or "close the one that is open". + /// + /// One after the other, which is what a first read and a test want. lists + /// beside a send instead, so a long one does not stop the window following the owner. + /// + /// + public bool Pump(out bool sent) => + List(SendPosted(out sent)); + + /// + /// Everything posted, in order, and what the last of it said. Null when nothing was. /// - public bool Pump(out bool sent) + string? SendPosted(out bool sent) { sent = false; string? message = null; @@ -61,6 +88,15 @@ public bool Pump(out bool sent) message = Send(command); } + return message; + } + + /// + /// Reads the queue back into the session, with being what a + /// finished send said, to arrive in the same mutation as the listing that shows its effect. + /// + bool List(string? message) + { if (!ViewerClient.TrySend(new(ViewerVerb.ListFull), out var response, port, Wait)) { return false; @@ -81,7 +117,7 @@ public bool Pump(out bool sent) var pending = InlineQueue.From(ViewerListing.Pending(response.Items)); var changes = ReadChanges(response); - host.Mutate(_ => ViewerSession.Sync(_, pending, changes, message)); + host.Mutate(_ => ViewerSession.Sync(_, pending, changes, message, response.Progress)); // The owner has no window of its own, so anything it wants raised, hidden or closed comes // back on the listing rather than being pushed at a port this process does not hold. @@ -119,11 +155,47 @@ public void Run(Cancel cancel) } } + /// + /// Sends go on a task of their own, and the listing carries on beside them. + /// + /// Send, then list, was one step, and an accept-all is one send for as long as the owner takes + /// to apply the whole queue. The window said "Waiting for the queue owner." over a list that + /// did not move for all of that, and then emptied at once. Listed alongside, the window follows + /// the owner as it goes - entries leaving as they land, and the owner's own count of how far + /// it has got in the status line. + /// + /// + /// Only this loop lists, and a finished send's message rides the first listing taken after it, + /// so the two still reach the session together, and a listing from partway through can never + /// land after the one that shows the result. + /// + /// void Pump(Cancel cancel) { + Task? sending = null; while (!cancel.IsCancellationRequested) { - if (!Pump(out var sent)) + string? message = null; + if (sending is { IsCompleted: true }) + { + message = sending.Result; + sending = null; + } + + if (sending is null && + !outbound.IsEmpty) + { + sending = Task.Run(() => SendPosted(out _), Cancel.None); + // Once it has finished rather than as it is finishing, so the pass it wakes finds + // it complete instead of waiting out another interval for the message + sending.ContinueWith( + _ => wake.Set(), + Cancel.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } + + if (!List(message)) { // The owner went away, so there is nothing left to display and no queue for this // window to be reopened from. @@ -135,13 +207,7 @@ void Pump(Cancel cancel) return; } - if (sent) - { - // Straight back round, so the window does not lag its own click by an interval. - continue; - } - - cancel.WaitHandle.WaitOne(Interval); + WaitHandle.WaitAny([cancel.WaitHandle, wake], Interval); } } @@ -149,7 +215,7 @@ string Send(Outbound command) { // The long wait matters most here: an accept is the command that takes ten seconds, and // failing it at three used to report the owner dead while it was mid apply. - if (!ViewerClient.TrySend(new(command.Verb, command.Key, command.Body), out var response, port, Wait)) + if (!ViewerClient.TrySend(new(command.Verb, command.Key, command.Body), out var response, port, SendWait)) { return "The queue owner is no longer running."; } diff --git a/src/DiffEngineViewer/ScreenBuilder.cs b/src/DiffEngineViewer/ScreenBuilder.cs index c8c426d5..14bfd256 100644 --- a/src/DiffEngineViewer/ScreenBuilder.cs +++ b/src/DiffEngineViewer/ScreenBuilder.cs @@ -125,7 +125,10 @@ static IReadOnlyList BuildQueue(SessionState state, int body, out int static IReadOnlyList