Skip to content

Synchronize SIGINT delivery in test_general_signal - #799

Open
ryanchou1994 wants to merge 2 commits into
amoffat:developfrom
ryanchou1994:test/synchronize-signal-delivery
Open

ryanchou1994 wants to merge 2 commits into
amoffat:developfrom
ryanchou1994:test/synchronize-signal-delivery

Conversation

@ryanchou1994

Copy link
Copy Markdown

test_general_signal currently relies on two-second sleeps in the child. If the output callback is delayed after reading 3, the child can print 4 before SIGINT arrives, breaking the expected 0, 1, 2, 3, 42, 43 output.

Wait for the handler's existing i = 42 update before printing the final two values. The wait uses a monotonic deadline and a short sleep; the handler only assigns the variable. A parent timeout also bounds the test if the child stops making progress. The change is confined to this test.

Validation:

  • A controlled three-second callback delay fails on the upstream version and passes with this change on macOS and Linux. Delays of zero and eight seconds also pass.
  • A signal arriving before the wait succeeds; no signal produces a timeout error after ten seconds.
  • macOS, Python 3.14.6: 188 passed, 2 skipped.
  • Linux, Python 3.14.7: poll 188 passed, 2 skipped; select 187 passed, 3 skipped; the root test passes with both backends. The changed test also passes under the C locale with both backends.
  • Ruff check and format pass using the version in uv.lock.

Refs #784.

Wait for the signal handler to update the child before checking its final output. Bound the child wait and the parent process without using a lock in the handler. Refs amoffat#784.
Copilot AI balanced review requested due to automatic review settings October 4, 2026 16:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
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.

2 participants