◐ Shell
reader mode source ↗
Skip to content

gh-107219: Fix concurrent.futures terminate_broken()#109244

Merged
vstinner merged 2 commits into
python:mainfrom
vstinner:cf_termine_broken
Sep 11, 2023
Merged

gh-107219: Fix concurrent.futures terminate_broken()#109244
vstinner merged 2 commits into
python:mainfrom
vstinner:cf_termine_broken

Conversation

@vstinner

@vstinner vstinner commented Sep 10, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Fix a race condition in concurrent.futures. When a process in the process pool was terminated abruptly (while the future was running or pending), close the connection write end. If the call queue is blocked on sending bytes to a worker process, closing the connection write end interrupts the send, so the queue can be closed.

Changes:

  • _ExecutorManagerThread.terminate_broken() now closes call_queue._writer.
  • multiprocessing PipeConnection.close() now interrupts WaitForMultipleObjects() in _send_bytes() by cancelling the overlapped operation.

@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 10, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 10, 2023
Closed
@vstinner

vstinner commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka @methane @ambv @gpshead @pitrou: Would you mind to have a look?

I would like to merge this fix as soon as possible since the bug #107219 is affecting very badly the Python workflow. The CI failure rate is very high because of this test_concurrent_futures.test_deadlock hang.

For now, I prefer to use WSA_OPERATION_ABORTED = 995 in Lib/multiprocessing/connection.py to ease backports. Later, I will try to add this constant somewhere :-) My first attempt to add it to the errno module didn't work (I didn't insist, I was working on the fix).

@vstinner

vstinner commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

With this change, I can no longer reproduce bug.

On my Windows VM which has 2 CPUs, I can easily reproduce the hang in around 30 seconds on the Python main branch:

  • Terminal 1: python -m test test_concurrent_futures.test_deadlock -m test.test_concurrent_futures.test_deadlock.ProcessPoolSpawnExecutorDeadlockTest.test_crash_big_data --forever -v --timeout=10
  • Terminal 2: python -m test -j2 -r

I stressed the test with:

  • Terminal 1, terminal 2 and terminal 3 (3 processes):

    • python -m test test_concurrent_futures.test_deadlock -m test.test_concurrent_futures.test_deadlock.ProcessPoolSpawnExecutorDeadlockTest.test_crash_big_data --forever -v --timeout=10

  • Terminal 4: python -m test test_concurrent_futures.test_deadlock -m test.test_concurrent_futures.test_deadlock.ProcessPoolSpawnExecutorDeadlockTest.test_crash_big_data --forever -v --timeout=30 -j2

  • Terminal 5: python -m test -j1 -r -u all

In 8 minutes, I failed to reproduce the bug anymore with this change.

Bonus: Moreover, I can no longer hang the test when I interrupt it with CTRL+C.

@vstinner

vstinner commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

Windows (x64) (pull_request) Successful

Oh! For the first time in like 2 weeks, test_concurrent_futures.test_deadlock did not hang in the GHA Windows x64 job!

Note: There are only these two unrelated failures:

2 re-run tests:
    test.test_asyncio.test_windows_events
    test.test_concurrent_futures.test_as_completed

These 2 tests passed when re-run in verbose mode (Result: FAILURE then SUCCESS).

@serhiy-storchaka serhiy-storchaka self-requested a review September 11, 2023 06:47
serhiy-storchaka
serhiy-storchaka approved these changes Sep 11, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Hide comment

LGTM.

But I have one suggestion and one question/suggestion.

@vstinner
pythongh-107219: Fix concurrent.futures terminate_broken() …
4c82464 
Fix a race condition in concurrent.futures. When a process in the
process pool was terminated abruptly (while the future was running or
pending), close the connection write end. If the call queue is
blocked on sending bytes to a worker process, closing the connection
write end interrupts the send, so the queue can be closed.

Changes:

* _ExecutorManagerThread.terminate_broken() now closes
  call_queue._writer.
* multiprocessing PipeConnection.close() now interrupts
  WaitForMultipleObjects() in _send_bytes() by cancelling the
  overlapped operation.
@vstinner
Remove PipeConnection.__init__() …
069fbfa 
Address Serhiy's review.
@vstinner vstinner enabled auto-merge (squash) September 11, 2023 07:48
@vstinner vstinner merged commit a9b1f84 into python:main Sep 11, 2023
@vstinner vstinner deleted the cf_termine_broken branch September 11, 2023 08:11
@miss-islington

miss-islington commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Sep 11, 2023

Copy link
Copy Markdown

There's a new commit after the PR has been approved.

@serhiy-storchaka: please review the changes made to this pull request.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2023
@vstinner @miss-islington
pythongh-107219: Fix concurrent.futures terminate_broken() (pythonGH-… …
34467dd 
…109244)

Fix a race condition in concurrent.futures. When a process in the
process pool was terminated abruptly (while the future was running or
pending), close the connection write end. If the call queue is
blocked on sending bytes to a worker process, closing the connection
write end interrupts the send, so the queue can be closed.

Changes:

* _ExecutorManagerThread.terminate_broken() now closes
  call_queue._writer.
* multiprocessing PipeConnection.close() now interrupts
  WaitForMultipleObjects() in _send_bytes() by cancelling the
  overlapped operation.
(cherry picked from commit a9b1f84)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot

bedevere-bot commented Sep 11, 2023

Copy link
Copy Markdown

GH-109254 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Sep 11, 2023
@bedevere-bot

bedevere-bot commented Sep 11, 2023

Copy link
Copy Markdown

GH-109255 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 11, 2023
@vstinner

vstinner commented Sep 11, 2023

Copy link
Copy Markdown
Member Author

PR merged, thanks for the review @serhiy-storchaka.

I wanted to merge this fix ASAP since it prevented to merge others PRs.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2023
@vstinner @miss-islington
pythongh-107219: Fix concurrent.futures terminate_broken() (pythonGH-… …
482d96f 
…109244)

Fix a race condition in concurrent.futures. When a process in the
process pool was terminated abruptly (while the future was running or
pending), close the connection write end. If the call queue is
blocked on sending bytes to a worker process, closing the connection
write end interrupts the send, so the queue can be closed.

Changes:

* _ExecutorManagerThread.terminate_broken() now closes
  call_queue._writer.
* multiprocessing PipeConnection.close() now interrupts
  WaitForMultipleObjects() in _send_bytes() by cancelling the
  overlapped operation.
(cherry picked from commit a9b1f84)

Co-authored-by: Victor Stinner <vstinner@python.org>
serhiy-storchaka
serhiy-storchaka reviewed Sep 11, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Hide comment

According to the sources of GetOverlappedResult() in _winapi.c, the only value of err can be ERROR_SUCCESS (0), ERROR_MORE_DATA, ERROR_OPERATION_ABORTED, ERROR_IO_INCOMPLETE.

@serhiy-storchaka

serhiy-storchaka commented Sep 11, 2023

Copy link
Copy Markdown
Member

Great work, @vstinner!

@vstinner vstinner mentioned this pull request Sep 11, 2023
Merged
vstinner added a commit that referenced this pull request Sep 11, 2023
@miss-islington @vstinner
[3.11] gh-107219: Fix concurrent.futures terminate_broken() (GH-109244)… …
2f8c80f 
… (#109255)

gh-107219: Fix concurrent.futures terminate_broken() (GH-109244)

Fix a race condition in concurrent.futures. When a process in the
process pool was terminated abruptly (while the future was running or
pending), close the connection write end. If the call queue is
blocked on sending bytes to a worker process, closing the connection
write end interrupts the send, so the queue can be closed.

Changes:

* _ExecutorManagerThread.terminate_broken() now closes
  call_queue._writer.
* multiprocessing PipeConnection.close() now interrupts
  WaitForMultipleObjects() in _send_bytes() by cancelling the
  overlapped operation.
(cherry picked from commit a9b1f84)

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner

vstinner commented Sep 11, 2023

Copy link
Copy Markdown
Member Author

According to the sources of GetOverlappedResult() in _winapi.c, the only value of err can be ERROR_SUCCESS (0), ERROR_MORE_DATA, ERROR_OPERATION_ABORTED, ERROR_IO_INCOMPLETE.

Well, if you're confident, you can modify the assert err == 0 in the code.

By the way, having nwritten, err = ov.GetOverlappedResult(True) in the finally: block sounds wrong to me. What if _winapi.WaitForMultipleObjects() raises an exception? Why is it important to call ov.GetOverlappedResult(True) in this case? But well, since I don't know the code, I prefer to not touch it!

Great work, @vstinner!

Thanks.

@vstinner vstinner mentioned this pull request Sep 22, 2023
Closed
Yhg1s pushed a commit that referenced this pull request Oct 2, 2023
@miss-islington @vstinner
[3.12] gh-107219: Fix concurrent.futures terminate_broken() (GH-109244)… …
1d8c18c 
… (#109254)

gh-107219: Fix concurrent.futures terminate_broken() (GH-109244)

Fix a race condition in concurrent.futures. When a process in the
process pool was terminated abruptly (while the future was running or
pending), close the connection write end. If the call queue is
blocked on sending bytes to a worker process, closing the connection
write end interrupts the send, so the queue can be closed.

Changes:

* _ExecutorManagerThread.terminate_broken() now closes
  call_queue._writer.
* multiprocessing PipeConnection.close() now interrupts
  WaitForMultipleObjects() in _send_bytes() by cancelling the
  overlapped operation.
(cherry picked from commit a9b1f84)

Co-authored-by: Victor Stinner <vstinner@python.org>
@colesbury colesbury mentioned this pull request Mar 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments

@serhiy-storchaka serhiy-storchaka

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @miss-islington @bedevere-bot @serhiy-storchaka @gpshead