Skip to content

gh-156070: Fix operator precedence in multiprocessing's batched Windows wait - #156071

Open
ekanshul wants to merge 3 commits into
python:mainfrom
ekanshul:fix-exhaustive-wait-precedence
Open

gh-156070: Fix operator precedence in multiprocessing's batched Windows wait#156071
ekanshul wants to merge 3 commits into
python:mainfrom
ekanshul:fix-exhaustive-wait-precedence

Conversation

@ekanshul

Copy link
Copy Markdown

In the Windows branch of multiprocessing.connection._exhaustive_wait() that handles more than 60 handles, the filter

L = [h for i, h in enumerate(L) if i > res[0] & i not in res]

parses as i > (res[0] & i) not in res because & binds tighter than the comparison operators, so signalled handles could stay in the list and unsignalled ones be dropped. Use and, as intended.

…tched wait

``i > res[0] & i not in res`` parses as ``i > (res[0] & i) not in res``
because ``&`` binds tighter than comparisons, so the Windows >60-handle
path of ``_exhaustive_wait`` kept the wrong handles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ekanshul
ekanshul requested a review from gpshead as a code owner August 19, 2026 21:00
@python-cla-bot

Copy link
Copy Markdown

The following commit authors need to sign the Contributor License Agreement:

CLA not signed

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If possible add a regression test

@@ -0,0 +1,3 @@
Fix :func:`multiprocessing.connection.wait` on Windows with more than 60
handles: the batched wait used ``&`` where ``and`` was meant when removing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not mention the & detail. It is an implementation detail.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, the NEWS entry now describes the user visible effect only:

Fix :func:multiprocessing.connection.wait on Windows when waiting on more than 60 objects: an object that was already ready could be reported more than once, and other ready objects could be left out of the result.

@bedevere-app

bedevere-app Bot commented Aug 19, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ekanshul

Copy link
Copy Markdown
Author

Added a regression test in TestWait.test_exhaustive_wait_more_than_60_handles. It creates 70 manual reset events, signals four of them spread across the list (indices 3, 17, 42 and 68, leaving index 0 unsignalled so the batched path is actually exercised), and checks that _exhaustive_wait returns exactly those four.

On the old code the filter kept the already signalled handles in the remaining list, so they were waited on again and the call returned [3, 17, 42, 68, 17, 42, 68]. It also dropped unsignalled handles that should have stayed in the list. The test is skipped off Windows, so the Windows CI job is what covers it.

I have made the requested changes; please review again

@bedevere-app

bedevere-app Bot commented Aug 20, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-app
bedevere-app Bot requested a review from picnixz August 20, 2026 19:11
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ekanshul

Copy link
Copy Markdown
Author

The Windows jobs went red on my previous push and I have just fixed the test that caused it. Two mistakes in it, both my own, the connection.py change is untouched:

  • I called _exhaustive_wait(events, 0). With a zero timeout BatchedWaitForMultipleObjects reports a timeout before its worker threads have run, so _exhaustive_wait took the except TimeoutError: return [] path and the test saw []. It now passes a real timeout.
  • I had signalled events [3, 17, 42, 68]. Since the batched wait reports only the lowest signalled handle of each 63 handle batch, whether the old code produced a wrong result depended on whether the second batch got to report before being cancelled, so the test could have passed against the unfixed code. Signalling event 0 pins the first reported index at 0, which makes i > res[0] & i not in res evaluate 0 not in res and discard every remaining handle, so the old behaviour is caught either way.

Sorry for the noise.

@picnixz

picnixz commented Aug 20, 2026

Copy link
Copy Markdown
Member

You need to sign the CLA as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants