Repository navigation
Conversation
Assisted-by: Codex:gpt-6
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe mmap implementation now validates mappings during buffer creation and manages mapping guards during close and resize operations. Tests cover closed mappings, exported child views, and Windows resize behavior. Changesmmap guard lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes mmap buffer exports synchronize with close and resize. No concrete merge-blocking risk was identified in the supplied context. The PR reports no test results, so the usual pre-merge checks still apply. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6b4352d2
ℹ️ 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".
|
|
||
| #[pymethod] | ||
| fn close(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<()> { | ||
| let mut mmap = zelf.mmap.lock(); |
There was a problem hiding this comment.
Reject exported close calls before taking the data lock
When one thread passes this mmap to _SSLSocket.write, crates/stdlib/src/openssl.rs:3073-3104 holds the mmap guard via data.borrow_buf() across a potentially unbounded allow_threads call. A concurrent mapped.close() now blocks here instead of observing the already-nonzero export count and raising BufferError; because this mutex wait remains attached to the interpreter, it can also participate in a stop-the-world deadlock. Keep an unlocked export-count fast path before this acquisition and recheck under the lock to preserve the race fix; check_resizeable needs the same ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, you're right. Fixed in aa7ac19. close() and check_resizeable() now reject existing exports before acquiring the mapping mutex, while retaining the check under the lock to prevent invalidating a concurrent export.
Both new regressions hold an actual borrowed mmap buffer across the operation. They failed before this change and now pass on Linux and Windows.
Assisted-by: Codex:gpt-6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa7ac19eb5
ℹ️ 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".
| with assert_raises(ValueError): | ||
| memoryview(mapped) |
There was a problem hiding this comment.
Remove new assertions disallowed by the test policy
This block adds new test setup, assertions, and data to a test file, but the repository's critical test policy permits test-file edits only for adding or removing expected-failure markers (and their associated TODO comments). The added snippet therefore needs to be removed or the repository policy changed before landing.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
Merging this PR will improve performance by 15.38%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | rustpython[frozenset.py] |
411.6 µs | 356.8 µs | +15.38% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing 1ndahous3:mmap_buffer_lifetime (aa7ac19) with main (54e47cd)
Footnotes
-
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Summary
AI assistance
Written with Codex (GPT-6), reviewed by a human before submission.
Summary by CodeRabbit