bpo-33929: multiprocessing: fix handle leak on race condition #7921

vstinner · 2018-06-26T00:10:24Z

Fix a race condition in Popen of
multiprocessing.popen_spawn_win32. The child process now duplicates
rhandle instead of "stealing" it.

Previously, rhandle was "stolen" by the child process, but it leaked
a handle if the child process has been terminated before it "stole"
the handle (and closed it in the parent process).

https://bugs.python.org/issue33929

vstinner · 2018-06-26T10:44:44Z

@pitrou: My first version was wrong, but I fixed it and now tests pass on AppVeyor and VSTS (Windows). Would you mind to review this change please? I'm not 100% sure that it's correct, but I'm now sure that the current code has a race condition :-)

pitrou · 2018-06-26T16:42:50Z

Lib/multiprocessing/reduction.py

So is this function still used?

It's part of the public API, no? At least, it's exported in all on Windows.

pitrou · 2018-06-26T16:43:11Z

Lib/multiprocessing/popen_spawn_win32.py

I'm not sure this comment is useful.

The first version of my patch tried to close sentinel as soon as the process completed. But it broke a test. Then I understood that it's public attribute, and so it cannot be closed here. For me it's non-obvious that close() cannot be closed "soon" (at least, not by Popen itself).

I don't know. The comment confuses me more than the situation it's supposed to comment :-)

pitrou · 2018-06-26T16:43:29Z

Lib/multiprocessing/popen_spawn_win32.py

s/it/the read end of pipe/

pitrou · 2018-06-26T16:43:40Z

Lib/multiprocessing/popen_spawn_win32.py

s/has been/had been/

pitrou · 2018-06-26T16:44:03Z

Lib/multiprocessing/popen_spawn_win32.py

"before it could steal the handle from the parent process"

pitrou · 2018-06-26T16:44:27Z

Lib/multiprocessing/reduction.py

A bit annoying. One argument is a process handle and the other is a pid?

steal_handle() also has a weird API, but I agree that it's not a good reason to continue that :-) I changed the parameter to source_process, and moved the pid => handle dance to the caller.

vstinner

I rebased my PR to be able to rewrite the commit message and update properly the NEWS entry.

vstinner · 2018-06-26T22:14:43Z

Lib/multiprocessing/popen_spawn_win32.py

vstinner · 2018-06-26T22:14:47Z

Lib/multiprocessing/popen_spawn_win32.py

vstinner · 2018-06-26T22:15:31Z

Lib/multiprocessing/reduction.py

steal_handle() also has a weird API, but I agree that it's not a good reason to continue that :-) I changed the parameter to source_process, and moved the pid => handle dance to the caller.

bpo-33929: Fix a race condition in Popen of multiprocessing.popen_spawn_win32. The child process now duplicates the read end of pipe instead of "stealing" it. Previously, the read end of pipe was "stolen" by the child process, but it leaked a handle if the child process had been terminated before it could steal the handle from the parent process.

vstinner · 2018-06-27T08:50:07Z

@pitrou: I fixed all the issues that you reported. Would you mind to review my updated PR? Thanks.

By the way, I found a similar but different bug: https://bugs.python.org/issue33966 But it's a different code path, so I think that it's fine to fix it in a different PR.

vstinner · 2018-06-27T08:59:23Z

I don't know. The comment confuses me more than the situation it's supposed to comment :-)

Ok, fair. I removed the comment.

pitrou · 2018-06-27T08:59:41Z

Thank you for unconfusing me :-)

pitrou

Thanks @vstinner !

vstinner · 2018-06-27T09:02:21Z

Thank you for unconfusing me :-)

LOL

miss-islington · 2018-06-27T09:40:27Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2018-06-27T09:41:41Z

GH-7960 is a backport of this pull request to the 3.7 branch.

…GH-7921) Fix a race condition in Popen of multiprocessing.popen_spawn_win32. The child process now duplicates the read end of pipe instead of "stealing" it. Previously, the read end of pipe was "stolen" by the child process, but it leaked a handle if the child process had been terminated before it could steal the handle from the parent process. (cherry picked from commit 2cc9d21) Co-authored-by: Victor Stinner <vstinner@redhat.com>

miss-islington · 2018-06-27T09:42:30Z

Sorry, @vstinner, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2cc9d21fffb8146d30e6fb4221e32410ba4b4ab7 3.6

bedevere-bot · 2018-06-27T09:46:46Z

GH-7961 is a backport of this pull request to the 3.6 branch.

pitrou · 2018-06-27T09:49:44Z

Victor, I'm skeptical about backporting this. It's not an important issue and we risk introducing regressions in a bugfix release. Let's make it a 3.8-only change?

vstinner · 2018-06-27T09:59:23Z

Victor, I'm skeptical about backporting this. It's not an important issue and we risk introducing regressions in a bugfix release. Let's make it a 3.8-only change?

I'm trying to understand why multiprocessing tests are failing randomly but also frequently, for one year (if not longer, 3 years?). Fixing race conditions should reduce random tests failures, but I'm not sure that leaking open Windows handles can explain these failures.

About regression: IMHO the new code is less fragile and should not have no race condition. But I'm ok to not backport this change, except if someone proves that this change helps to make tests more stable :-)

Fix a race condition in Popen of multiprocessing.popen_spawn_win32. The child process now duplicates the read end of pipe instead of "stealing" it. Previously, the read end of pipe was "stolen" by the child process, but it leaked a handle if the child process had been terminated before it could steal the handle from the parent process. (cherry picked from commit 2cc9d21) Co-authored-by: Victor Stinner <vstinner@redhat.com>

vstinner · 2018-06-27T10:05:11Z

I rejected my 3.6 backport: PR 7961.

But @miss-islington merged its 3.7 backport: PR #7960 :-(

@pitrou: Should I revert the backport to 3.7? Maybe it's fine for the 3.7.1, since it's newer?

…GH-7921)" This reverts commit 8b1ebcd.

vstinner · 2018-06-27T10:09:17Z

@pitrou: Should I revert the backport to 3.7? Maybe it's fine for the 3.7.1, since it's newer?

Hum, since I already found a bug in my fix, I proposed a PR to revert the 3.7 backport: PR #7963.

…GH-7921)" (GH-7963) This reverts commit 8b1ebcd.

…ondition (GH-7921)" (GH-7963)" This reverts commit 7b5856e.

…GH-7921)" This reverts commit 8b1ebcd.

vstinner added needs backport to 3.6 labels Jun 26, 2018

vstinner requested review from applio and pitrou June 26, 2018 00:10

bedevere-bot added the awaiting merge label Jun 26, 2018

the-knights-who-say-ni added the CLA signed label Jun 26, 2018

vstinner changed the title ~~multiprocessing: fix handle leak on race condition~~ bpo-33929: multiprocessing: fix handle leak on race condition Jun 26, 2018

pitrou reviewed Jun 26, 2018

View reviewed changes

Lib/multiprocessing/popen_spawn_win32.py Outdated

Copy link

Member

pitrou Jun 26, 2018

Choose a reason for hiding this comment

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

s/it/the read end of pipe/

pitrou reviewed Jun 26, 2018

View reviewed changes

vstinner commented Jun 26, 2018

View reviewed changes

Remove comment which confuses Antoine :-)

9a14d5d

pitrou approved these changes Jun 27, 2018

View reviewed changes

vstinner removed the needs backport to 2.7 label Jun 27, 2018

vstinner merged commit 2cc9d21 into python:master Jun 27, 2018

bedevere-bot removed the awaiting merge label Jun 27, 2018

vstinner deleted the mp_spawn_win32 branch June 27, 2018 09:40

bedevere-bot removed the needs backport to 3.7 label Jun 27, 2018

miss-islington assigned vstinner Jun 27, 2018

bedevere-bot removed the needs backport to 3.6 label Jun 27, 2018

vstinner mentioned this pull request Jun 27, 2018

[3.6] bpo-33929: multiprocessing: fix handle leak on race condition (GH-7921) #7961

Closed

vstinner added a commit that referenced this pull request Jun 27, 2018

Revert "bpo-33929: multiprocessing: fix handle leak on race condition (…

7dac610

…GH-7921)" This reverts commit 8b1ebcd.

vstinner added a commit that referenced this pull request Jun 27, 2018

Revert "bpo-33929: multiprocessing: fix handle leak on race condition (…

7b5856e

…GH-7921)" (GH-7963) This reverts commit 8b1ebcd.

terryjreedy added a commit that referenced this pull request Jun 27, 2018

Revert "Revert "bpo-33929: multiprocessing: fix handle leak on race c…

28bd988

…ondition (GH-7921)" (GH-7963)" This reverts commit 7b5856e.

terryjreedy added a commit that referenced this pull request Jun 27, 2018

Revert "bpo-33929: multiprocessing: fix handle leak on race condition (…

f331080

…GH-7921)" This reverts commit 8b1ebcd.

richardsheridan mentioned this pull request Jan 29, 2024

Multiprocessing PipeConnection handles leak on failed subprocess spawn #114720

Open

Uh oh!

bpo-33929: multiprocessing: fix handle leak on race condition #7921

bpo-33929: multiprocessing: fix handle leak on race condition #7921

Uh oh!

Conversation

vstinner commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

pitrou commented Jun 27, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

miss-islington commented Jun 27, 2018

Uh oh!

bedevere-bot commented Jun 27, 2018

Uh oh!

miss-islington commented Jun 27, 2018

Uh oh!

bedevere-bot commented Jun 27, 2018

Uh oh!

pitrou commented Jun 27, 2018

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

vstinner commented Jun 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Jun 26, 2018 •

edited

Loading