fix: make manual query cancellable via dedicated CancellationTokenSource#70
fix: make manual query cancellable via dedicated CancellationTokenSource#70
Conversation
Manual queries (OnServiceQueryRequested) previously used CancellationToken.None, making them impossible to cancel. This caused three issues: 1. Unresponsive services (e.g. Ollama) could not be cancelled by the user 2. Page/window close did not stop in-flight manual queries 3. New main queries did not cancel stale manual queries for old text Add _manualQueryCts field (scheme B) with clear ownership: - OnServiceQueryRequested: creates CTS, cancels previous, disposes in finally - StartQueryAsync: cancels manual queries when new main query starts - CleanupResourcesAsync: cancels manual queries on page/window close https://claude.ai/code/session_01GXde8bGRRhP3p6X9LVCVuW
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
This PR makes manual per-service queries (triggered by OnServiceQueryRequested) cancellable by introducing a dedicated CancellationTokenSource for manual queries, and ensuring manual queries are canceled on new main queries and during window/page cleanup.
Changes:
- Add
_manualQueryCtsto track and cancel in-flight manual queries. - Plumb the manual cancellation token through language detection and translation calls.
- Cancel manual queries when starting a new main query and during cleanup/shutdown.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| dotnet/src/Easydict.WinUI/Views/MiniWindow.xaml.cs | Introduces _manualQueryCts, passes cancellation tokens into manual query execution, and cancels manual queries on new main queries/cleanup. |
| dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs | Mirrors the same manual-query cancellation model for the main page workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| oldCts?.Dispose(); | ||
|
|
There was a problem hiding this comment.
Disposing the previous manual query CTS here can break an in-flight manual query that is still using its CancellationToken (many APIs register callbacks and may throw ObjectDisposedException if the source is disposed). Since the previous OnServiceQueryRequested invocation owns/disposes its CTS in its finally block, prefer to only Cancel() the old CTS here and let the owning invocation dispose it.
| oldCts?.Dispose(); |
| // Cancel any in-flight manual queries (stale text) | ||
| var oldManualCts = Interlocked.Exchange(ref _manualQueryCts, null); | ||
| oldManualCts?.Cancel(); | ||
| oldManualCts?.Dispose(); |
There was a problem hiding this comment.
Disposing the manual CTS here can race with the in-flight manual query that’s still using the token, which can surface as ObjectDisposedException rather than a clean cancellation. Recommend only Cancel()ing the exchanged CTS and letting the owning OnServiceQueryRequested invocation dispose it in its finally.
| oldManualCts?.Dispose(); | |
| // Do not dispose here; the owning OnServiceQueryRequested invocation | |
| // is responsible for disposing the CTS in its finally block. |
| // Cancel any in-flight manual queries | ||
| var manualCts = Interlocked.Exchange(ref _manualQueryCts, null); | ||
| manualCts?.Cancel(); | ||
| manualCts?.Dispose(); |
There was a problem hiding this comment.
Disposing the manual CTS during cleanup can race with a still-running manual query that’s using the token, potentially causing ObjectDisposedException inside awaited operations. Prefer to exchange+Cancel() but leave disposal to the owning OnServiceQueryRequested invocation’s finally block.
| manualCts?.Dispose(); |
| oldManualCts?.Cancel(); | ||
| oldManualCts?.Dispose(); |
There was a problem hiding this comment.
Disposing the exchanged manual CTS here can race with the in-flight manual query that is still using its token, producing ObjectDisposedException instead of a clean cancellation. Recommend only Cancel() here and let OnServiceQueryRequested dispose its CTS in finally.
| oldManualCts?.Cancel(); | |
| oldManualCts?.Dispose(); | |
| // Only cancel here; disposal is handled in the manual query's finally block | |
| oldManualCts?.Cancel(); |
| @@ -43,6 +43,7 @@ private struct POINT | |||
| // Owned by StartQueryAsync() - only that method creates and disposes via its finally block. | |||
| // Other code may Cancel() but must NOT Dispose(). | |||
| private CancellationTokenSource? _currentQueryCts; | |||
There was a problem hiding this comment.
_currentQueryCts has an ownership comment explaining who may Dispose(); _manualQueryCts doesn’t. Since multiple methods Cancel()/Exchange this field, please add a short ownership/lifetime comment (e.g., owned by OnServiceQueryRequested, others must not Dispose) to prevent future misuse.
| private CancellationTokenSource? _currentQueryCts; | |
| private CancellationTokenSource? _currentQueryCts; | |
| // Owned by the manual query workflow (creator method owns Dispose()). | |
| // Other code may Cancel()/exchange this field but must NOT Dispose(). |
| // Cancel any in-flight manual queries | ||
| var manualCts = Interlocked.Exchange(ref _manualQueryCts, null); | ||
| manualCts?.Cancel(); | ||
| manualCts?.Dispose(); |
There was a problem hiding this comment.
Disposing the manual CTS during cleanup can race with the still-running manual query, potentially causing ObjectDisposedException inside awaited operations that use the token. Prefer to exchange+Cancel() to signal shutdown, but leave disposal to the owning OnServiceQueryRequested finally block (similar to the _currentQueryCts ownership pattern).
| // Cancel any in-flight manual queries | |
| var manualCts = Interlocked.Exchange(ref _manualQueryCts, null); | |
| manualCts?.Cancel(); | |
| manualCts?.Dispose(); | |
| // Cancel any in-flight manual queries; disposal is handled by the owner | |
| var manualCts = Interlocked.Exchange(ref _manualQueryCts, null); | |
| manualCts?.Cancel(); |
| // Create new CTS, cancel any previous manual query | ||
| var cts = new CancellationTokenSource(); | ||
| var oldCts = Interlocked.Exchange(ref _manualQueryCts, cts); | ||
| oldCts?.Cancel(); | ||
| oldCts?.Dispose(); | ||
|
|
There was a problem hiding this comment.
Disposing the previous manual query CTS here can break an in-flight manual query that is still using its CancellationToken (ObjectDisposedException during token registrations). Since the previous OnServiceQueryRequested invocation disposes its CTS in finally, only Cancel() the old CTS here and let the owner dispose it.
| // Create new CTS, cancel any previous manual query | |
| var cts = new CancellationTokenSource(); | |
| var oldCts = Interlocked.Exchange(ref _manualQueryCts, cts); | |
| oldCts?.Cancel(); | |
| oldCts?.Dispose(); | |
| // Create new CTS, cancel any previous manual query (owner will dispose in its own finally) | |
| var cts = new CancellationTokenSource(); | |
| var oldCts = Interlocked.Exchange(ref _manualQueryCts, cts); | |
| oldCts?.Cancel(); |
| catch (OperationCanceledException) | ||
| { | ||
| serviceResult.IsLoading = false; | ||
| serviceResult.IsStreaming = false; |
There was a problem hiding this comment.
On cancellation, MarkQueried() has already run but no Result/Error is set, so manual-query services can end up stuck (HasQueried=true prevents QueryRequested from firing again on re-expand). Consider resetting the ServiceQueryResult on cancellation (or restoring HasQueried=false) so users can retry the manual query.
| serviceResult.IsStreaming = false; | |
| serviceResult.IsStreaming = false; | |
| serviceResult.HasQueried = false; |
| } | ||
|
|
||
| // Create new CTS, cancel any previous manual query | ||
| var cts = new CancellationTokenSource(); |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
| } | ||
|
|
||
| // Create new CTS, cancel any previous manual query | ||
| var cts = new CancellationTokenSource(); |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
Manual queries (OnServiceQueryRequested) previously used CancellationToken.None,
making them impossible to cancel. This caused three issues:
Add _manualQueryCts field (scheme B) with clear ownership:
https://claude.ai/code/session_01GXde8bGRRhP3p6X9LVCVuW