Repository navigation
Validate RLock acquisition arguments before reentrant success - #9035
Conversation
Extract the Python 3.14-compatible timeout validation order from RustPython#8954. Share validation with Lock and run it before increasing the RLock recursion count. Keep canonical general-lock coverage and add only owned-lock validation regressions to the existing thread snippet. Assisted-by: OpenAI Codex:model identifier unavailable
|
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 41 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. Comment |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6848a7530
ℹ️ 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".
| from testutils import assert_raises | ||
|
|
||
| # Reentrant calls must validate arguments before incrementing the count. | ||
| for error, kwargs in ( |
There was a problem hiding this comment.
This loop adds new test logic and assertions to a test file, but the repository policy says test files may only be changed to remove an expected-failure marker when the test passes or to add one when it cannot be fixed. This addition is neither permitted form, so remove this test-file change rather than expanding the snippet.
AGENTS.md reference: AGENTS.md:L275-L279
Useful? React with 👍 / 👎.
| if !args.blocking && timeout != -1.0 { | ||
| return Err(vm.new_value_error("can't specify a timeout for a non-blocking call")); | ||
| } | ||
| if args.blocking && timeout < 0.0 && timeout != -1.0 { |
There was a problem hiding this comment.
When an already-owned RLock receives acquire(blocking=False, timeout=_thread.TIMEOUT_MAX + 1), CPython 3.14 raises OverflowError, but this branch runs before the upper-bound check and returns ValueError; +inf follows the same path, while -inf is also misclassified by the following negative-value branch. Because the commit newly routes reentrant acquisition through this validator, out-of-range calls still get the wrong exception; validate both bounds and finiteness before applying the blocking/sign rules.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against exact CPython 3.14.7 and native main. Follow-up draft #9061 checks the signed nanosecond range before the acquire-specific restrictions, preserving the separate platform limit and -1 sentinel. The modified snippet, unchanged reacquire/timeout tests, both snippet pytest entries, bounded recursion-state probes, normal hooks, and VM/CAPI Clippy pass. The adjacent pre-existing fractional TIMEOUT_MAX discrepancy is explicitly documented and unchanged.
2 participants
Validate
RLock.acquire()arguments before the reentrant fast path. Invalid non-blocking/time-out combinations, negative timeouts, and excessive timeouts must raise without increasing the recursion count, matching CPython 3.14.Extracted from #8954. The shared argument check also preserves ordinary
Lockvalidation. Adds a focused owned-lock regression to the existing thread snippet; canonical tests retain the general lock coverage.Validation:
AI-assisted implementation, tests, and review with OpenAI Codex. Kept as a draft for CI and maintainer review.