Repository navigation
Fix asyncio callback ordering and reentrant completion - #9673
youknowdot wants to merge 1 commit into
Conversation
Share Future and Task callback handling, preserve callback lifetimes across reentrant removal, and validate Future exception completion boundaries. Assisted-by: OpenAI Codex:model version not exposed
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afc385fd7e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cb0 = self.fut_callback0.read().clone(); | ||
| if let Some(cb0) = cb0 { | ||
| let cmp = vm.identical_or_equal(&cb0, &func)?; | ||
| if cmp { | ||
| *zelf.fut_callback0.write() = None; | ||
| *zelf.fut_context0.write() = None; | ||
| let cmp = vm.identical_or_equal(&cb0, func); |
There was a problem hiding this comment.
AGENTS.md requires: "Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code." This refactor removes // Clone to release lock before comparison (which may run Python code), although the shared helper still relies on precisely that sequence here. The comment documents why equality must run without a field lock, so retain it in the extracted implementation rather than deleting it with the duplicate code.
AGENTS.md reference: AGENTS.md:L165-L165
Useful? React with 👍 / 👎.
| drop(callback); | ||
| let context = self.fut_context0.write().take(); |
There was a problem hiding this comment.
If a pending future owns the last reference to its first callback, removing it via a distinct equal object can now run its __del__ at drop(callback), since the temporary cb0 reference was already dropped. If that finalizer calls add_done_callback(replacement, context=saved_context) and no callback list exists, registration repopulates both fut_callback0 and fut_context0. The following take() then clears the replacement's context, so completion runs the surviving callback in the completer's context instead of its registered context. The parent kept cb0 alive until both old fields were cleared. Take both old fields before dropping either reference so finalization cannot consume newly registered state.
Useful? React with 👍 / 👎.
Summary
Share Future/Task callback registration, removal and scheduling instead of maintaining duplicate implementations.
call_soonobserve and remove later callbacks before detaching their list.Production Rust only: 153 additions and 292 deletions. Existing argument-binding structs are reused.
Reference
Matches CPython 3.15 callback scheduling, callback removal, and exception completion.
Validation
cargo check -p rustpython-vm -p rustpython-host_env -p rustpython-stdlib --features rustpython-stdlib/threading --locked --offline -j1cargo clippy -p rustpython-vm -p rustpython-host_env -p rustpython-stdlib --features rustpython-stdlib/threading --locked --offline -j1 -- -D warningsNative RustPython runtime tests, full workspace/C-API tests and cross-platform validation were not run. This does not redesign Task stepping/initialization or claim complete parity for generic list finalization.
AI assistance: implementation, source comparison and review were assisted by OpenAI Codex; model version is not exposed.