Skip to content

Commit c31508e

Browse files
crutkasCopilot
andcommitted
PT Run: cancel previous query on empty pluginQueryPairs + defer CTS dispose on CancelAndWait timeout
Two fixes spotted while doing a follow-up read of the cancellation refactor for an unrelated review: 1. MainViewModel.cs — previous session was only cancelled when pluginQueryPairs had at least one entry. A non-empty QueryText whose QueryBuilder.Build() returns an empty dictionary (e.g. all global plugins disabled, no keyword match) fell through both cancellation paths and left the in-flight session running. The pre-refactor code cancelled _updateSource unconditionally before the Count check; this restores that invariant by hoisting previousSession.Cancel() + DisposeWhenComplete() out of the Count > 0 block while keeping _currentSession bound to the just-cancelled session (so late IResultUpdated events continue to suppress via their captured token rather than falling back to CancellationToken.None). 2. QuerySession.CancelAndWait — on timeout, the CTS was disposed unconditionally while the tracked task may still have been running. That violates the refactor's own "tasks never observe a disposed CTS" invariant: any future plugin path that touches token WaitHandles (Register, WaitOne) after timeout would crash with ObjectDisposedException. The fix gates dispose on the completed flag and, on timeout, defers disposal via ContinueWith with DenyChildAttach (mirroring DisposeWhenComplete's pattern). Also softens the QuerySession remarks: the "structural guarantee" only holds for callers that capture Token into a local. The class can't prevent a future author from re-reading _currentSession.Token inside a body, which would reintroduce the original bug. Added regression test CancelAndWait_DoesNotDisposeCtsWhileTaskStillRuns covering the new deferred-disposal behavior; all 10 Wox.Test QuerySessionTest cases pass. --- ADO: https://microsoft.visualstudio.com/DefaultCollection/OS/_workitems/edit/55588441/ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2eb9fab commit c31508e

3 files changed

Lines changed: 74 additions & 6 deletions

File tree

src/modules/launcher/PowerLauncher/ViewModel/MainViewModel.cs

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -803,12 +803,23 @@ private void QueryResults(bool? delayedExecution)
803803
});
804804

805805
_currentSession = currentSession;
806+
}
806807

807-
if (previousSession != null)
808-
{
809-
previousSession.Cancel();
810-
_ = previousSession.DisposeWhenComplete();
811-
}
808+
// Cancel the prior in-flight query for ANY non-empty query, even one
809+
// that produced zero plugin pairs (e.g. all globals disabled + no
810+
// keyword match, so QueryBuilder.Build returns an empty dictionary).
811+
// The pre-refactor code cancelled _updateSource unconditionally before
812+
// the Count check; this restores that invariant so a non-empty query
813+
// whose Build() yields zero pairs still tears down the previous
814+
// fan-out instead of leaking it. Cancel() and DisposeWhenComplete()
815+
// are idempotent, and Token is a stored snapshot, so leaving
816+
// _currentSession bound to the just-cancelled session keeps late
817+
// IResultUpdated events suppressed via their captured token rather
818+
// than falling back to CancellationToken.None and slipping through.
819+
if (previousSession != null)
820+
{
821+
previousSession.Cancel();
822+
_ = previousSession.DisposeWhenComplete();
812823
}
813824
}
814825
else

src/modules/launcher/PowerLauncher/ViewModel/QuerySession.cs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ namespace PowerLauncher.ViewModel
2929
/// captures the token as a local cannot be tricked by a later
3030
/// <c>_currentSession = new QuerySession()</c> reassignment into observing a
3131
/// fresh (non-cancelled) token.
32+
/// <para>
33+
/// Note: "structural" applies only to callers that capture <see cref="Token"/>
34+
/// into a local before passing it into a task body. <see cref="QuerySession"/>
35+
/// cannot prevent a future author from re-reading <c>_currentSession.Token</c>
36+
/// inside a loop / continuation, which would reintroduce the original bug
37+
/// shape — code review of every observer site is still required.
3238
/// </para>
3339
/// </remarks>
3440
internal sealed class QuerySession : IDisposable
@@ -147,7 +153,24 @@ public bool CancelAndWait(TimeSpan timeout)
147153

148154
if (Interlocked.Exchange(ref _disposed, 1) == 0)
149155
{
150-
_cts.Dispose();
156+
if (completed)
157+
{
158+
_cts.Dispose();
159+
}
160+
else
161+
{
162+
// The tracked task outlived our timeout; disposing _cts under
163+
// it would risk ObjectDisposedException if the task later
164+
// touches token WaitHandles. Defer disposal until the task
165+
// actually completes. DenyChildAttach so an attached child
166+
// task cannot delay disposal past the parent's completion.
167+
_ = Completion.ContinueWith(
168+
static (_, state) => ((CancellationTokenSource)state).Dispose(),
169+
_cts,
170+
CancellationToken.None,
171+
TaskContinuationOptions.DenyChildAttach,
172+
TaskScheduler.Default);
173+
}
151174
}
152175

153176
return completed;

src/modules/launcher/Wox.Test/QuerySessionTest.cs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,40 @@ public void CancelAndWait_ReturnsFalseWhenTaskExceedsTimeout()
135135
session.Completion.Wait(WaitBudget);
136136
}
137137

138+
[TestMethod]
139+
public void CancelAndWait_DoesNotDisposeCtsWhileTaskStillRuns()
140+
{
141+
// Regression: CancelAndWait used to dispose the underlying CTS unconditionally
142+
// on timeout, leaving any still-running task body holding a disposed source —
143+
// a future code path that touches token WaitHandles (e.g. Register, WaitOne)
144+
// would crash with ObjectDisposedException, violating the PR's own
145+
// "tasks never observe a disposed CTS" invariant. The fix defers disposal
146+
// until the task actually completes.
147+
var release = new ManualResetEventSlim(initialState: false);
148+
CancellationToken capturedToken = default;
149+
150+
var session = QuerySession.Start(token =>
151+
{
152+
capturedToken = token;
153+
return Task.Run(() => release.Wait(WaitBudget));
154+
});
155+
156+
bool completed = session.CancelAndWait(TimeSpan.FromMilliseconds(50));
157+
Assert.IsFalse(completed, "Sanity: task must outlive the timeout.");
158+
159+
// While the task is still running with the CTS undisposed,
160+
// CancellationToken.Register must succeed (would throw ODE if disposed).
161+
using (capturedToken.Register(() => { }))
162+
{
163+
// no-op; just proves Register didn't throw
164+
}
165+
166+
// Allow the task to complete; the deferred ContinueWith should then dispose
167+
// the CTS, and a subsequent Register attempt is allowed to throw ODE.
168+
release.Set();
169+
session.Completion.Wait(WaitBudget);
170+
}
171+
138172
[TestMethod]
139173
public void Dispose_IsIdempotent_AndSafeAfterCancel()
140174
{

0 commit comments

Comments
 (0)