Repository navigation
Restore interrupted with-enter coverage with pending exception support - #9497
youknowone merged 2 commits into
Conversation
Deliver VM-owned pending exception classes at bytecode safepoints and clear their roots before terminal owner teardown. Add the native SelfInterruptingContextManager fixture and restore the canonical test_with regression via update_lib. Assisted-by: OpenAI Codex: model version unavailable
|
Caution Review failedThe pull request is closed.
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. |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_with.py dependencies: dependent tests: (no tests depend on with) [ ] test: cpython/Lib/test/test_cmd_line.py (TODO: 12) dependencies: dependent tests: (no tests depend on cmd_line) Legend:
|
Merging this PR will not alter performance
Comparing Footnotes
|
| const SPECIALIZATION_THRESHOLD: usize = 2; | ||
|
|
||
| #[pyattr] | ||
| #[pyclass(name = "SelfInterruptingContextManager", module = "_testcapi")] |
There was a problem hiding this comment.
| #[pyclass(name = "SelfInterruptingContextManager", module = "_testcapi")] | |
| #[pyclass(module = "_testcapi")] |
you don't need to repeat name
Apply the review request to avoid repeating SelfInterruptingContextManager. Keep the required bare name marker: the current pyclass macro resolves it to the identical Rust identifier. Assisted-by: OpenAI Codex: model version unavailable
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: 33de790f50
ℹ️ 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".
| def test_exit_called_after_interrupt(self): | ||
| cm = SelfInterruptingContextManager() | ||
| self.assertFalse(cm.within()) |
There was a problem hiding this comment.
Obtain a policy exception before restoring this test body
The root AGENTS.md states that “the only acceptable modifications to test files” are the specified expected-failure decorator and TODO-comment changes. Adding this test body and its import exceeds that restriction, and the guidance documents no exemption for restoring canonical CPython tests. Obtain an explicit upstream-restoration exemption or keep the test-file edits within the permitted forms.
AGENTS.md reference: AGENTS.md:L280-L282
Useful? React with 👍 / 👎.
|
|
||
| /// Called only at bytecode safepoints, after pending signals and GC. | ||
| pub(crate) fn raise_async_exception(&self) -> PyResult<()> { | ||
| let Some(exception) = self.async_exception.take() else { |
There was a problem hiding this comment.
Prevent profiling callbacks from consuming the pending interrupt
With a Python callback such as sys.setprofile(lambda *args: None) installed, the successful native __enter__ call invokes its c_return profiling callback before returning to the with frame (protocol/callable.rs:119–123). The callback's first bytecode safepoint consumes the pending exception here, but _trace_event_inner discards profiling callback errors (protocol/callable.rs:409–419). Consequently, the KeyboardInterrupt disappears, profiling is disabled, and the restored test reaches “No exception raised.” Defer delivery during instrumentation callbacks or propagate the callback error so profiling cannot silently cancel the interrupt.
Useful? React with 👍 / 👎.
Summary
Restore the missing
test_with.InterruptDuringEnterregression from pinned CPythond220409a13b77de7a6c30abe16ea90db01528edcthrough the realupdate_libworkflow. The canonical class/import are restored unchanged; existing test markers remain unchanged.The fixture needs genuine deferred delivery rather than an immediate
KeyboardInterruptfrom__enter__. This adds:PyThreadState_SetAsyncExcsupport.SelfInterruptingContextManagerfixture and explicit root clearing on supported terminal thread/interpreter paths. Reusableenter/runscopes do not clear pending state.Lifecycle and API review points
DroptoInterpreterrestricts moving its publicglobal_statefield out directly. In-repository uses were checked; external consumers were not.ThreadedVirtualMachine.vmis not a public external field.Cell<bool>state on the non-SyncVM. A future cross-thread API needs a separate synchronization/wakeup design. There is still an extra plain flag load per opcode; no performance-neutrality claim is made.Validation
Local validation used base
c2bb450316db3ce3dc4ddc6bcde9f3059ee4e90c. Main advanced to0072412e9523b37f7c7254c9ceb3468d5041c454(#9495) during publication; that newer merge composition is left to PR CI, not claimed as locally retested.The local build/runtime checks below cover original implementation commit
770484ecfd47e6b3af502cc72ce7819fa16e594e: checks preceded that metadata-only commit, and normal hooks left all 3,779 build-bound source files unchanged.Review follow-up
33de790f50417c21c6a67d3a5a3a0661a53d86baonly changes the class attribute to#[pyclass(name, module = "_testcapi")]. The shared macro name resolver returns the same string from the Rust identifier as from the previous literal; registration and type metadata are unchanged by source inspection. Normal hooks passed. Local compilation/runtime were not repeated for that one-line follow-up; its own PR CI is tracked separately.On the original implementation source tree:
cargo check, strictcargo clippy -- -D warnings, and CLI build passed withthreading,stdlib,stdio,importlib,host_env,doc,ucd_3_2_0and actual VM opt-level 3.threading; the actual VM compiler invocation had no threading cfg.InterruptDuringEnter.test_exit_called_after_interruptran once against the candidate and once against a fresh pinned CPython build containing the real_testinternalcapifixture. Both ran exactly one test with zero failures, errors, skips, expected failures, or unexpected successes. Inputs were hash-bound, output matched, caches stayed empty, and owned process groups were cleaned up.This was a bounded Linux single-method comparison, not full
test_with, workspace/C-API suites, cross-platform runtime testing, a benchmark, or validation of every generic normalization/finalizer/TLS corner case. No baseline crash probes, OS-signal injection, or cross-thread stress tests were run.There is file-level overlap with #9143 in
_testinternalcapi.rsand VM registration-area files, but this does not duplicate its_testcapiregistration work. #9407's C-APItp_newwork remains separate. Neither owner's branch was modified.AI assistance
Implementation, source analysis, independent agent review, guarded validation, and this description were assisted by OpenAI Codex (model version unavailable).