Repository navigation
impl _queue module + Update multiprocessing to 3.14.5 - #7994
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a stdlib ChangesSimpleQueue Module Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/stdlib/src/_queue.rs`:
- Around line 152-169: The busy-wait in the queue consumer loop (the block in
the method that calls empty(), get_inner(), and checks start_time/end_time)
should be replaced with a proper blocking wait using a Condvar: protect the
queue state with a Mutex (or use parking_lot::Mutex) and a Condvar (or
parking_lot::Condvar), change the consumer path to call condvar.wait() /
condvar.wait_timeout_until() in a loop that re-checks !self.empty() and the
deadline (using start_time/end_time) before returning unsafe { self.get_inner()
} or Err(empty_error(vm)); update the producer path (e.g., put/insert path) to
notify_one()/notify_all() on the same Condvar after pushing an item so waiters
wake; ensure the timeout branch uses the condvar timeout API instead of busy
polling so the thread sleeps until signaled or deadline expires.
- Around line 164-168: The timeout check in the queue get logic is inverted: in
the block that matches (Some(start), Some(end)) (inside the get method / loop),
change the condition from start.elapsed() < end to start.elapsed() >= end so the
code only returns Err(empty_error(vm)) after the timeout has elapsed (i.e., when
elapsed >= end) instead of immediately; update the conditional covering
(Some(start), Some(end)) accordingly so waiting continues while elapsed < end
and only raises Empty once elapsed >= end.
- Around line 50-70: The current get/get_inner pattern has a race: get calls
self.empty() under a read lock then get_inner takes a separate write lock and
does unsafe pop_front(). Change get_inner (and its callers) to acquire the write
lock once, perform an atomic check-and-pop, and return Option<PyObjectRef>
(removing unsafe and unwrap_unchecked); update get to handle the Option result
instead of relying on a prior empty() check; also fix the doc comment wording
from "Called must ensure" to "Caller must ensure" and remove the unsafe block
and its SAFETY claim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e0f001b4-21df-46f5-9393-0ebb38eeb378
📒 Files selected for processing (2)
crates/stdlib/src/_queue.rscrates/stdlib/src/lib.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/_queue.rs (1)
137-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent panic when converting
timeouttoDurationinSimpleQueue.get()
TimeoutSecondsonly rejectsNaN, butSimpleQueue.get()converts the value withDuration::from_secs_f64(v); Rust’sfrom_secs_f64panics forinf/non-finite and out-of-range values, which can crash the VM instead of raising a Python error. Use the stableDuration::try_from_secs_f64and raiseOverflowErroron failure.🛡️ Proposed fix
match timeout.map(|v| v.to_secs_f64()) { Some(v) if v < 0.0 => { return Err(vm.new_value_error("'timeout' must be a non-negative number")); } - Some(v) => Some(Duration::from_secs_f64(v)), + Some(v) => match Duration::try_from_secs_f64(v) { + Ok(d) => Some(d), + Err(_) => return Err(vm.new_overflow_error("timeout value is out of range")), + }, None => None, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/_queue.rs` around lines 137 - 144, SimpleQueue.get() currently uses Duration::from_secs_f64(v) which can panic for non-finite or out-of-range inputs; change this to use Duration::try_from_secs_f64(v) and map a failure to returning an OverflowError via the VM (e.g. vm.new_overflow_error) so the error is raised to Python instead of crashing the VM; adjust the match branch that handles Some(v) to call try_from_secs_f64, return Ok(Some(duration)) on success or return Err(vm.new_overflow_error("timeout out of range")) on failure, keeping the existing None and negative-value checks intact and referencing TimeoutSeconds and SimpleQueue.get for locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/stdlib/src/_queue.rs`:
- Around line 137-144: SimpleQueue.get() currently uses
Duration::from_secs_f64(v) which can panic for non-finite or out-of-range
inputs; change this to use Duration::try_from_secs_f64(v) and map a failure to
returning an OverflowError via the VM (e.g. vm.new_overflow_error) so the error
is raised to Python instead of crashing the VM; adjust the match branch that
handles Some(v) to call try_from_secs_f64, return Ok(Some(duration)) on success
or return Err(vm.new_overflow_error("timeout out of range")) on failure, keeping
the existing None and negative-value checks intact and referencing
TimeoutSeconds and SimpleQueue.get for locating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 29c5157f-838c-4f6d-b303-3684d7bb17ca
📒 Files selected for processing (1)
crates/stdlib/src/_queue.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/_queue.rs (1)
213-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
unwrap()panics on ordering comparisons (<,>,<=,>=).
identical_optimizationreturnsNonefor ordering operations when objects differ. Using.unwrap()will panic if someone attemptsqueue1 < queue2. CPython'sSimpleQueuereturnsNotImplementedfor such comparisons.🐛 Proposed fix: return NotImplemented for non-identity comparisons
impl Comparable for PySimpleQueue { fn cmp( zelf: &Py<Self>, other: &PyObject, op: PyComparisonOp, _vm: &VirtualMachine, ) -> PyResult<PyComparisonValue> { - Ok(op.identical_optimization(zelf, other).unwrap().into()) + Ok(op.identical_optimization(zelf, other) + .map(PyComparisonValue::Implemented) + .unwrap_or(PyComparisonValue::NotImplemented)) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/_queue.rs` around lines 213 - 222, The Comparable impl for PySimpleQueue calls identical_optimization(...).unwrap(), which panics for ordering ops when it returns None; change cmp in the impl for PySimpleQueue to call op.identical_optimization(zelf, other) and if it returns Some(value) return Ok(value.into()), otherwise return Ok(PyComparisonValue::NotImplemented) (i.e., do not unwrap but map the None case to PyComparisonValue::NotImplemented) so ordering comparisons return NotImplemented instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/stdlib/src/_queue.rs`:
- Around line 213-222: The Comparable impl for PySimpleQueue calls
identical_optimization(...).unwrap(), which panics for ordering ops when it
returns None; change cmp in the impl for PySimpleQueue to call
op.identical_optimization(zelf, other) and if it returns Some(value) return
Ok(value.into()), otherwise return Ok(PyComparisonValue::NotImplemented) (i.e.,
do not unwrap but map the None case to PyComparisonValue::NotImplemented) so
ordering comparisons return NotImplemented instead of panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: de1e12f3-8ccc-4ad9-b3c3-e0f262844d7e
📒 Files selected for processing (1)
crates/stdlib/src/_queue.rs
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/multiprocessing dependencies:
dependent tests: (11 tests)
Legend:
|
|
ref #3608 |
|
@roryyorke I'd appreciate your review, you seem more knowledgeable than me on |
@ShaharNaveh I'm afraid I'm no expert. I am happy to review, but can only do that next weekend (6 June or 7 June). The only advice I have is to give some thought to the reentrancy of SimpleQueue.put and .get. The comments on the CPython issue leading to SimpleQueue (see my comment on gh-3608 for links) say the reason for a C implementation was to use the GIL as a lock around the queue put and get operations. I read this as "bytecode atomicity", that is, a builtin method call is an atomic VM operation, so there cannot be a |
_queue module_queue module + Update multiprocessing to 3.14.5
Summary
Summary by CodeRabbit
_queuemodule providing a thread-safeSimpleQueuewithput/put_nowait,get/get_nowait(optional blocking with timeout),empty, andqsize.Emptyexception available from the module;SimpleQueuesupports truthiness checks and PEP-compliant generic type subscripting._queueis registered during stdlib initialization so it’s available at startup.