Skip to content

Fix cancellation event lifetime in Windows handle waits - #3570

Closed
harshitdeora wants to merge 1 commit into
python-trio:mainfrom
harshitdeora:fix/windows-wait-cancellation-handle
Closed

harshitdeora wants to merge 1 commit into
python-trio:mainfrom
harshitdeora:fix/windows-wait-cancellation-handle

Conversation

@harshitdeora

Copy link
Copy Markdown

Fixes #1271.

Cancelling WaitForSingleObject() currently signals and immediately closes its internal cancellation event. The abandoned worker may not have entered WaitForMultipleObjects() yet, or may still be using that handle. The new deterministic tests reproduce the premature close with real Windows events. This affects the reliability of Windows handle waits, including subprocess waits.

Coordinate cleanup between the task and worker with a lock and completion flags. The last participant closes the event. A dedicated per-call capacity limiter also identifies cancellation or thread-start failure before a worker was launched, so those paths close the event as well. Cleanup is explicit and does not depend on garbage collection. The caller still owns the original handle.

This follows the two-participant cleanup direction discussed in #1271. Earlier related proposals #1834 and #2374 are closed; this change keeps the existing thread-per-wait implementation.

Validation on Windows / CPython 3.14.0:

  • Exact final test_wait_for_object.py on base ec8d886f: 2 failed, 8 passed. Both failures are the forced cancellation orderings.
  • Updated code, including slow wait tests: 10 passed. Coverage includes cancellation before the OS wait, cancellation after the OS wait but before worker return, normal completion, cancellation before worker launch, worker errors, and thread startup failures. The tests check event validity and exactly one close without forcing garbage collection.
  • Related wait/subprocess/pipe/thread tests with slow tests enabled: 94 passed, 9 skipped.
  • Broader pytest src --skip-optional-imports: 808 passed, 85 skipped, 3 xfailed, 3 failed. All three failures are in test_testing_raisesgroup.py; the same three reproduce on unchanged base code with pytest 9.2.0. They concern matcher formatting, outside this change.
  • Ruff 0.17.3, Black 26.10.1, mypy with --platform win32 on both changed Python files, and git diff --check pass.
  • A local 300-wait round-trip diagnostic, three samples per fresh process in base/head/head/base order, gave process medians of 0.199 / 0.226 / 0.205 / 0.252 ms per wait. These noisy local measurements overlap; they are not a portable performance estimate.

Other Python versions, PyPy, and free-threaded Python were not tested locally. Hosted CI has not been claimed as passed.

@codecov

codecov Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.17837%. Comparing base (7a43473) to head (f776110).
⚠️ Report is 1 commits behind head on main.

❌ Your project check has failed because the head coverage (98.17837%) is below the target coverage (100.00000%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                 Coverage Diff                  @@
##                 main       #3570         +/-   ##
====================================================
- Coverage   100.00000%   98.17837%   -1.82163%     
====================================================
  Files             128         128                 
  Lines           19550       20037        +487     
  Branches         1330        1397         +67     
====================================================
+ Hits            19550       19672        +122     
- Misses              0         358        +358     
- Partials            0           7          +7     
Files with missing lines Coverage Δ
src/trio/_tests/test_wait_for_object.py 100.00000% <100.00000%> (ø)
src/trio/_wait_for_object.py 100.00000% <100.00000%> (ø)

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@A5rocks

A5rocks commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

I don't think these tests look good and I don't want to review an LLM generated PR that's complicated.

@A5rocks A5rocks closed this Nov 1, 2026
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.

Race condition in trio.hazmat.WaitForSingleObject

2 participants