Add rx.event arg to cancel previous background tasks#6793
Add rx.event arg to cancel previous background tasks#6793tim-haselhoff wants to merge 11 commits into
Conversation
benedikt-bartscher
left a comment
There was a problem hiding this comment.
Looks good to me, thanks Tim!
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge; the cancellation bookkeeping is correct, cleanup is properly guarded, and the new behavior is well-tested. The cancellation bookkeeping handles all edge cases correctly — task already done when superseded, task replaced before its No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "Merge branch 'main' into cancel-previous..." | Re-trigger Greptile |
…cessor.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
TODO: also check behaviour when running this within another event. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95fd0ceda4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| existing = self._tasks.get(existing_txid) | ||
| if existing is not None and not existing.done(): | ||
| existing.cancel() |
There was a problem hiding this comment.
Preserve task context before cancelling queued background runs
On Python 3.12+, when two matching cancel_previous_task background events are enqueued back-to-back, _process_queue can reach this existing.cancel() before the first task has ever run _process_event_queue_entry and set EventContext to entry.ctx. _finish_task then reads the task's original queue-processor context via task.get_context(), so it does not pop the first txid from _tasks or settle that EventFuture; callers waiting on the first future can hang and the task entry leaks. Store the event context on every task before it can be cancelled, as the pre-3.12 path already does.
Useful? React with 👍 / 👎.
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
After these steps, you're ready to open a pull request.