Skip to content

Commit 39cd4ff

Browse files
committed
Give a remote discard the time an accept gets
Discard and DiscardAll used ViewerClient.ShortTimeout, which is 500ms and exists for calls on a clock - the tray's scan timer, where waiting the full timeout would let callbacks outlast their own period. Discarding is not one of those: it comes from the menu or a hot key, and it no longer runs on the UI thread. Meanwhile the owner answering it may be inside InlineApplier, which waits up to ten seconds on its cross process mutex. So half a second turned a busy owner into "The snapshot viewer is not running." for a single discard. DiscardAll was worse, because its result was dropped. Tracker.Clear emptied its own snapshot list regardless, so "Discard (n)" reported success while the owner had never received the message - and everything reappeared on the next scan two seconds later. It now returns the outcome, and Clear only forgets what it holds when the owner confirms. Both wait on acceptWait, the same fifteen seconds an accept gets, for the same reason.
1 parent 0b71112 commit 39cd4ff

6 files changed

Lines changed: 41 additions & 10 deletions

File tree

src/DiffEngineTray.Tests/OwnedInlineHostTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ public async Task DiscardAllEmptiesTheQueue()
315315
owner.Queue();
316316
owner.Queue(@"c:\repo\OtherTests.cs", 7);
317317

318-
owner.Host.DiscardAll();
318+
owner.Host.DiscardAll(out _);
319319

320320
await Assert.That(owner.Host.List()).IsEmpty();
321321
}

src/DiffEngineTray.Tests/StubInlineHost.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,10 @@ public bool AcceptAll(out string? message)
5959
return AcceptAllSucceeds;
6060
}
6161

62-
public void DiscardAll()
62+
public bool DiscardAll(out string? message)
6363
{
64+
message = null;
65+
return true;
6466
}
6567

6668
public void Focus(PendingSnapshot snapshot)

src/DiffEngineTray/IInlineHost.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ interface IInlineHost
2626

2727
bool Discard(PendingSnapshot snapshot, out string? message);
2828
bool AcceptAll(out string? message);
29-
void DiscardAll();
29+
/// <summary>
30+
/// False when the queue owner could not be asked, so a caller clearing its own state knows not
31+
/// to.
32+
/// </summary>
33+
bool DiscardAll(out string? message);
3034

3135
/// <summary>
3236
/// Bring the window forward on this item, launching one if there is none.

src/DiffEngineTray/OwnedInlineHost.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,12 @@ public bool AcceptAll(out string? message)
143143
}
144144
}
145145

146-
public void DiscardAll() =>
147-
((IQueueOwner) this).DiscardAll();
146+
public bool DiscardAll(out string? message)
147+
{
148+
message = ((IQueueOwner) this).DiscardAll();
149+
// Owned in this process, so there is nobody to fail to reach
150+
return true;
151+
}
148152

149153
public void Focus(PendingSnapshot snapshot) =>
150154
Show(WindowCommand.Focus, snapshot.Key);

src/DiffEngineTray/RemoteInlineHost.cs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,14 @@ public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message)
9696
: AcceptOutcome.Applied;
9797
}
9898

99+
/// <summary>
100+
/// On <see cref="acceptWait"/>, not the short timeout. Discarding is not a clock driven call
101+
/// - it comes from the menu or a hot key - and the owner answering it may be busy inside
102+
/// InlineApplier, which waits up to ten seconds on its cross process mutex. Half a second
103+
/// turned a busy owner into "The snapshot viewer is not running."
104+
/// </summary>
99105
public bool Discard(PendingSnapshot snapshot, out string? message) =>
100-
Send(ViewerVerb.Discard, snapshot.Key, ViewerClient.ShortTimeout, out message);
106+
Send(ViewerVerb.Discard, snapshot.Key, acceptWait, out message);
101107

102108
/// <summary>
103109
/// True only when the queue is empty afterwards, for the reason <see cref="Accept"/> gives —
@@ -110,8 +116,14 @@ public bool AcceptAll(out string? message) =>
110116
Send(ViewerVerb.AcceptAll, null, acceptWait, out message) &&
111117
List().Count == 0;
112118

113-
public void DiscardAll() =>
114-
Send(ViewerVerb.DiscardAll, null, ViewerClient.ShortTimeout, out _);
119+
/// <summary>
120+
/// As <see cref="Discard"/>, and the outcome is returned rather than dropped. Discarded on a
121+
/// busy owner used to do nothing at all while Tracker.Clear went ahead and emptied its own
122+
/// snapshot list, so "Discard (n)" reported success and everything reappeared on the next
123+
/// scan two seconds later.
124+
/// </summary>
125+
public bool DiscardAll(out string? message) =>
126+
Send(ViewerVerb.DiscardAll, null, acceptWait, out message);
115127

116128
public void Focus(PendingSnapshot snapshot) =>
117129
Send(ViewerVerb.Focus, snapshot.Key, ViewerClient.ShortTimeout, out _);

src/DiffEngineTray/Tracker.cs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -671,8 +671,17 @@ public void Clear()
671671
{
672672
((ITrackedFiles) this).DiscardAll();
673673

674-
inline.DiscardAll();
675-
snapshots = [];
674+
// Only forget the cached snapshots when the owner actually discarded them. It used to be
675+
// cleared regardless, so a discard the owner never received still emptied the menu - and
676+
// everything came back on the next scan two seconds later
677+
if (inline.DiscardAll(out var message))
678+
{
679+
snapshots = [];
680+
}
681+
else
682+
{
683+
Log.Error(message ?? "Could not discard the pending snapshots.");
684+
}
676685
}
677686

678687
/// <summary>

0 commit comments

Comments
 (0)