Repository navigation
Fix cancellation event lifetime in Windows handle waits - #3570
Closed
harshitdeora wants to merge 1 commit into
Closed
harshitdeora wants to merge 1 commit into
harshitdeora wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ 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
🚀 New features to boost your workflow:
|
Contributor
|
I don't think these tests look good and I don't want to review an LLM generated PR that's complicated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1271.
Cancelling
WaitForSingleObject()currently signals and immediately closes its internal cancellation event. The abandoned worker may not have enteredWaitForMultipleObjects()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:
test_wait_for_object.pyon baseec8d886f: 2 failed, 8 passed. Both failures are the forced cancellation orderings.pytest src --skip-optional-imports: 808 passed, 85 skipped, 3 xfailed, 3 failed. All three failures are intest_testing_raisesgroup.py; the same three reproduce on unchanged base code with pytest 9.2.0. They concern matcher formatting, outside this change.--platform win32on both changed Python files, andgit diff --checkpass.Other Python versions, PyPy, and free-threaded Python were not tested locally. Hosted CI has not been claimed as passed.