⚡ Sangsakala Proxy 🏠 Home
Skip to content

impl _queue module + Update multiprocessing to 3.14.5 - #7994

Merged
youknowone merged 30 commits into
RustPython:mainfrom
ShaharNaveh:queue-module
Jun 1, 2026
Merged

youknowone merged 30 commits into
RustPython:mainfrom
ShaharNaveh:queue-module

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented May 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • New Features
    • Added a _queue module providing a thread-safe SimpleQueue with put/put_nowait, get/get_nowait (optional blocking with timeout), empty, and qsize.
    • Empty exception available from the module; SimpleQueue supports truthiness checks and PEP-compliant generic type subscripting.
    • _queue is registered during stdlib initialization so it’s available at startup.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 29, 2026 •

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a stdlib _queue module providing an Empty exception and a SimpleQueue class backed by a thread-safe VecDeque with Condvar, offering empty(), qsize(), put/put_nowait, get/get_nowait (blocking/timeout), arg parsing, behaviors, and stdlib registration.

Changes

SimpleQueue Module Implementation

Layer / File(s) Summary
Exception and storage structure
crates/stdlib/src/_queue.rs
Defines PyEmptyError mapped to Python Empty, empty_error helper, and PySimpleQueue using a mutex-protected VecDeque<PyObjectRef> plus Condvar, with get_inner() that pops front and shrinks capacity when occupancy <25%.
Method argument parsing
crates/stdlib/src/_queue.rs
Adds FromArgs structs for put and get methods handling compatibility block/timeout fields and typed TimeoutSeconds parsing for get.
Queue methods and constructor
crates/stdlib/src/_queue.rs
Implements SimpleQueue constructor, empty(), qsize(), put()/put_nowait() (enqueue + notify), and get()/get_nowait() with non-blocking and blocking/timeout behavior and wires Constructor to PySimpleQueue::new().
Python-visible behaviors
crates/stdlib/src/_queue.rs
Implements truthiness (AsNumber boolean), identity-based comparison (Comparable::cmp via identical_optimization), and repr for SimpleQueue.
Module declaration and registration
crates/stdlib/src/lib.rs
Adds mod _queue; and registers _queue::module_def(ctx) in stdlib_module_defs so the module participates in multi-phase stdlib initialization.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A queue that hops with VecDeque in tow,
Locks keep the carrots safe row after row,
When shelves grow sparse they shrink with a sigh,
Empty is raised if no treats pass by,
The rabbit nods — the queue is ready to go!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title mentions implementing _queue module which matches the primary changes (237 lines adding SimpleQueue implementation), but also references 'Update multiprocessing to 3.14.5' which is not evident in the provided file summaries. Clarify whether multiprocessing updates are included in this changeset, or revise the title to focus solely on the primary _queue module implementation.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1385c4e and 372d8ca.

📒 Files selected for processing (2)
  • crates/stdlib/src/_queue.rs
  • crates/stdlib/src/lib.rs

Comment thread crates/stdlib/src/_queue.rs Resolved
Comment thread crates/stdlib/src/_queue.rs Resolved
Comment thread crates/stdlib/src/_queue.rs Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Prevent panic when converting timeout to Duration in SimpleQueue.get()

TimeoutSeconds only rejects NaN, but SimpleQueue.get() converts the value with Duration::from_secs_f64(v); Rust’s from_secs_f64 panics for inf/non-finite and out-of-range values, which can crash the VM instead of raising a Python error. Use the stable Duration::try_from_secs_f64 and raise OverflowError on 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

📥 Commits

Reviewing files that changed from the base of the PR and between 372d8ca and ac3aa7a.

📒 Files selected for processing (1)
  • crates/stdlib/src/_queue.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_optimization returns None for ordering operations when objects differ. Using .unwrap() will panic if someone attempts queue1 < queue2. CPython's SimpleQueue returns NotImplemented for 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

📥 Commits

Reviewing files that changed from the base of the PR and between f8357c3 and c45f058.

📒 Files selected for processing (1)
  • crates/stdlib/src/_queue.rs

@github-actions

github-actions Bot commented May 30, 2026 •

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/multiprocessing
[x] test: cpython/Lib/test/test_multiprocessing_fork
[ ] test: cpython/Lib/test/test_multiprocessing_forkserver (TODO: 10)
[ ] test: cpython/Lib/test/test_multiprocessing_spawn (TODO: 13)
[x] test: cpython/Lib/test/test_multiprocessing_main_handling.py
[x] test: cpython/Lib/test/_test_multiprocessing.py (TODO: 8)

dependencies:

  • multiprocessing

dependent tests: (11 tests)

  • multiprocessing: test_asyncio test_compileall test_concurrent_futures test_fcntl test_genericalias test_logging test_memoryview test_multiprocessing_main_handling test_re test_socket
    • concurrent.futures.process: test_concurrent_futures

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@ShaharNaveh
ShaharNaveh marked this pull request as draft May 30, 2026 14:41
@youknowone

Copy link
Copy Markdown
Member

ref #3608

@ShaharNaveh

Copy link
Copy Markdown
Contributor Author

@roryyorke I'd appreciate your review, you seem more knowledgeable than me on _queue's internals

@roryyorke

Copy link
Copy Markdown
Contributor

I'd appreciate your review, you seem more knowledgeable than me on _queue's internals

@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 __del__ or weakref callback in the same thread while the put or get are in progress. If that is correct, and a similar argument holds for RustPython builtin methods, it seems likely that the reentrancy requirement will be satisfied.

@ShaharNaveh ShaharNaveh changed the title impl _queue module impl _queue module + Update multiprocessing to 3.14.5 Jun 1, 2026
@ShaharNaveh
ShaharNaveh marked this pull request as ready for review June 1, 2026 13:08
@youknowone
youknowone merged commit 3e1c3bc into RustPython:main Jun 1, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants