`_queue`: check for pending signals during blocking `get()` by sigmaith · Pull Request #8252 · RustPython/RustPython

sigmaith · 2026-07-11T06:34:19Z

Closes _queue.SimpleQueue.get() doesn't check for pending signals while blocked #8250
This PR follows our AI policy

Summary

queue.SimpleQueue.get() blocked via a single uninterrupted parking_lot::Condvar wait, so a Python-level signal handler never ran until the wait itself returned -- check_signals() is only invoked from the bytecode dispatch loop, and this native wait never passed through it. CPython avoids this by chunking its equivalent blocking wait (PyThread_acquire_lock_timed).

This PR caps each wait at a short interval and calls check_signals() between chunks, only giving up once the caller's real deadline has actually elapsed. The mutex guard is dropped before check_signals() runs, since a signal handler that calls back into the same queue on the same thread would otherwise deadlock trying to relock a mutex it's still
holding (a self-deadlock I hit and fixed while developing this).

[Before]

[After]

Test plan

Added extra_tests/snippets/stdlib_queue_signal.py: arms a SIGALRM well before a helper thread unblocks the queue, and asserts the handler ran near the alarm instead of only once get() returned. Verified it fails with an AssertionError against the pre-fix code and passes after the fix.
cargo run -- -m test test_queue -j 1: 162 passed, 6 skipped (unrelated, memory-gated).
cargo clippy / cargo fmt --check: clean.
Manually compared against real CPython on the same machine to confirm the expected (responsive) behavior.

AI disclosure

This change was developed with assistance from Claude Code(claude-sonnet-5): investigating the root cause, drafting the fix andregression test, and iterating after I found a self-deadlock in an early
version of the fix. I reviewed the diff, ran the build/lints/tests myself, and verified the fix and regression test against both RustPython and real CPython before opening this PR.

Assisted-by: Claude Code:claude-sonnet-5

Summary by CodeRabbit

Bug Fixes
- Improved signal responsiveness for queue.SimpleQueue.get() / get_nowait() while waiting for items.
- Queue waits now periodically check for pending signals, allowing signal handlers to run promptly and ensuring signal-related errors are properly surfaced.
Tests
- Added a regression test that schedules an OS signal during a blocked get() call, confirms the call unblocks as expected, and verifies the signal handler runs within a reasonable time (skips on platforms without the required signal support).

coderabbitai · 2026-07-11T06:34:54Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 84a6b41f-e1e4-45bb-bbe0-0ac9840073ff

📥 Commits

Reviewing files that changed from the base of the PR and between 256f201 and a8ae1e3.

📒 Files selected for processing (2)

crates/stdlib/src/_queue.rs
extra_tests/snippets/stdlib_queue_signal.py

🚧 Files skipped from review as they are similar to previous changes (2)

extra_tests/snippets/stdlib_queue_signal.py
crates/stdlib/src/_queue.rs

📝 Walkthrough

Walkthrough

SimpleQueue semaphore waits now use bounded intervals to process signals while blocked, propagate signal-handler errors, and preserve timeout behavior. A regression script verifies prompt SIGALRM handling before a worker unblocks get().

Changes

SimpleQueue signal handling

Layer / File(s)	Summary
Bounded semaphore acquisition `crates/stdlib/src/_queue.rs`	Threaded semaphore waits are chunked by `SIGNAL_CHECK_INTERVAL`, return `PyResult<bool>`, check signals after releasing the wait guard, and update `get` and `get_nowait` callers.
Signal responsiveness regression test `extra_tests/snippets/stdlib_queue_signal.py`	Schedules `SIGALRM`, blocks `SimpleQueue.get`, unblocks it from a worker thread, and asserts the handler ran before `get` returned.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MainThread
  participant SimpleQueue
  participant Semaphore
  participant SignalHandler
  participant WorkerThread
  MainThread->>SimpleQueue: get()
  SimpleQueue->>Semaphore: acquire()
  loop bounded wait
    Semaphore->>Semaphore: wait up to SIGNAL_CHECK_INTERVAL
    Semaphore->>SignalHandler: vm.check_signals()
  end
  SignalHandler-->>Semaphore: handler completes
  WorkerThread->>SimpleQueue: put("unblock")
  SimpleQueue-->>MainThread: return "unblock"

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: making blocking `_queue.SimpleQueue.get()` check for pending signals.
Linked Issues check	✅ Passed	The changes address `#8250` by chunking waits, calling `check_signals()`, and adding a regression test for responsive signal handling.
Out of Scope Changes check	✅ Passed	The PR stays focused on signal responsiveness in `_queue.SimpleQueue.get()` and its regression test, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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.}

coderabbitai

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)
254-268: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the non-blocking get path with the semaphore.

get(block=False) bypasses self.sem.acquire(...), while get_nowait() consumes a permit before popping from buf. That leaves the semaphore count ahead of the buffer after a non-blocking get, so a later blocking get() can consume a stale permit and then raise Empty on an empty queue. get(block=False) and get_nowait() should use the same path.
🤖 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 254 - 268, The non-blocking get
path does not synchronize with the semaphore, causing permit and buffer counts
to diverge. Update the `get` method’s `block == false` handling to call the same
semaphore-acquisition logic as `get_nowait`, preserving the existing empty-queue
error behavior; use `get_nowait` or a shared helper to ensure both paths consume
a permit before removing from `self.buf`.

🤖 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 254-268: The non-blocking get path does not synchronize with the
semaphore, causing permit and buffer counts to diverge. Update the `get`
method’s `block == false` handling to call the same semaphore-acquisition logic
as `get_nowait`, preserving the existing empty-queue error behavior; use
`get_nowait` or a shared helper to ensure both paths consume a permit before
removing from `self.buf`.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4d087bfc-0d93-42ab-a54a-88410b6fb0f5

📥 Commits

Reviewing files that changed from the base of the PR and between 9c064c1 and 3409f2f.

📒 Files selected for processing (2)

crates/stdlib/src/_queue.rs
extra_tests/snippets/stdlib_queue_signal.py

youknowone · 2026-07-11T07:02:57Z

+    /// Wait chunk size so blocking waits stay responsive to signals, mirroring
+    /// CPython's `PyThread_acquire_lock_timed`.
+    #[cfg(feature = "threading")]
+    const SIGNAL_CHECK_INTERVAL: Duration = Duration::from_millis(50);


does this line has CPython reference?

No CPython citation for 50ms, It was empirical choice.
CPython doesn't chunk either; it makes one sem_timedwait call and relies on EINTR.
parking_lot's condvar doesn't expose signal interruptions to us the way CPython does, So I think polling is the workaround. Comment updated to reflect this.

Semaphore::acquire() waited on the condvar in a single uninterrupted call, so a Python-level signal handler never ran until the wait itself returned. check_signals() is only invoked from the bytecode dispatch loop, and this native wait never passed through it -- CPython chunks its equivalent blocking wait for the same reason. Cap each wait at a short interval and call check_signals() between chunks, only giving up once the caller's real deadline has actually elapsed. The mutex guard must be dropped before check_signals() runs: a signal handler that calls back into the same queue on the same thread would otherwise try to relock a mutex it's still holding and deadlock against itself. Add a regression snippet that arms a SIGALRM well before a helper thread unblocks the queue, and asserts the handler ran near the alarm instead of only once get() returned. Refs RustPython#8250 Assisted-by: Claude

The old comment claimed this mirrors CPython's PyThread_acquire_lock_timed, but CPython doesn't chunk its wait either -- it makes one sem_timedwait call and relies on the OS reporting EINTR. parking_lot's Condvar has no equivalent signal to surface, so polling is a workaround for that gap, not a mirror of CPython's approach. Assisted-by: Claude

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

youknowone reviewed Jul 11, 2026

View reviewed changes

youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 12, 2026

sigmaith added 2 commits July 12, 2026 22:11

sigmaith force-pushed the fix/queue-blocking-signals branch from 256f201 to a8ae1e3 Compare July 12, 2026 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`_queue`: check for pending signals during blocking `get()`#8252

`_queue`: check for pending signals during blocking `get()`#8252
sigmaith wants to merge 2 commits into
RustPython:mainfrom
sigmaith:fix/queue-blocking-signals

sigmaith commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

youknowone Jul 11, 2026

Uh oh!

sigmaith Jul 12, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sigmaith commented Jul 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

AI disclosure

Summary by CodeRabbit

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

sigmaith Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sigmaith commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

sigmaith Jul 12, 2026 •

edited

Loading