Repository navigation
Validate lock timeout range before acquire restrictions - #9061
Conversation
Preserve CPython signed nanosecond conversion precedence for reentrant and nonblocking acquisition, without changing the platform timeout maximum. Assisted-by: OpenAI Codex:model identifier unavailable
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change gives out-of-range timeouts the intended overflow precedence while preserving the sentinel and platform-limit checks. No concrete merge-blocking risk remains; proceed with normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: validating lock timeout range before applying acquire restrictions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
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: 33ff5abcf2
ℹ️ 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".
| (ValueError, {"blocking": False, "timeout": 1}), | ||
| (ValueError, {"timeout": -2}), | ||
| (OverflowError, {"timeout": _thread.TIMEOUT_MAX + 1}), | ||
| (OverflowError, {"blocking": False, "timeout": 2**63 / 1e9}), |
There was a problem hiding this comment.
These new timeout rows change the data that drives an existing test, but this repository's critical test-code policy explicitly forbids modifying test data and limits test-file changes to adding or removing expected-failure markers; remove these rows from this file before the change can comply.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
2 participants
Summary
Follow up on the range-precedence review of merged #9035: #9035 (comment).
Check whether a timeout fits signed nanoseconds before applying the nonblocking and negative-timeout restrictions. This makes out-of-range values and infinities raise
OverflowErrorbefore those argument-combination errors, including reentrant RLock acquisition. The implementation adds five lines to the existing shared validator; the existing Python regression table gains focused range cases.The signed conversion limit is distinct from the later platform maximum. In particular, Windows values outside its positive timeout maximum can still fit signed nanoseconds; negative values in that range must retain
ValueError. The-1sentinel, NaN handling, accepted timeout values, locking implementation and recursion accounting are unchanged.Exact reference sources:
Scope and validation
This is a narrow review correction, not a rewrite of timeout conversion. The pre-existing comparison against the floored public
TIMEOUT_MAXstill rejects some fractional values that CPython accepts; that adjacent discrepancy is unchanged.Independent source review passed, including signed endpoint asymmetry and the separate Windows limit.
Normal commit hooks passed.
Exact CPython 3.14.7 passed the modified full thread snippet and 40 bounded reference probes using already-owned RLocks.
Native build and the complete modified thread snippet passed.
Unchanged
CRLockTests.test_reacquireandCRLockTests.test_timeoutpassed.Both snippet pytest entries passed under the exact 3.14.7 test runner.
Forty bounded native probes preserved recursion state; all targeted cases match the reference. Only the two documented pre-existing fractional-maximum differences remain in that matrix.
VM and CAPI Clippy with
-D warningspassed.No full-workspace or Windows-runtime pass is claimed.
Summary by CodeRabbit