◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 2 commits into
python:masterfrom
vstinner:mp_spawn_win32
Jun 27, 2018
Merged

bpo-33929: multiprocessing: fix handle leak on race condition#7921
vstinner merged 2 commits into
python:masterfrom
vstinner:mp_spawn_win32

Conversation

@vstinner

@vstinner vstinner commented Jun 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 vstinner requested review from applio and pitrou June 26, 2018 00:10
@vstinner vstinner changed the title multiprocessing: fix handle leak on race condition Jun 26, 2018
@vstinner

vstinner commented Jun 26, 2018

Copy link
Copy Markdown
Member Author

@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 :-)

vstinner
vstinner commented Jun 26, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hide comment

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

@vstinner
multiprocessing: fix handle leak on race condition …
a03d662 
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

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

@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

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

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

Ok, fair. I removed the comment.

@pitrou

pitrou commented Jun 27, 2018

Copy link
Copy Markdown
Member

Thank you for unconfusing me :-)

pitrou
pitrou approved these changes Jun 27, 2018
View reviewed changes

@pitrou pitrou 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

Thanks @vstinner !

@vstinner

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

Thank you for unconfusing me :-)

LOL

@vstinner vstinner merged commit 2cc9d21 into python:master Jun 27, 2018
@miss-islington

miss-islington commented Jun 27, 2018

Copy link
Copy Markdown
Contributor

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!

@vstinner vstinner deleted the mp_spawn_win32 branch June 27, 2018 09:40
@bedevere-bot

bedevere-bot commented Jun 27, 2018

Copy link
Copy Markdown

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

14 hidden items Load more…
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 27, 2018
@vstinner @miss-islington
bpo-33929: multiprocessing: fix handle leak on race condition (python… …
8986866 
…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

miss-islington commented Jun 27, 2018

Copy link
Copy Markdown
Contributor

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

bedevere-bot commented Jun 27, 2018

Copy link
Copy Markdown

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

@pitrou

pitrou commented Jun 27, 2018

Copy link
Copy Markdown
Member

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

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

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 :-)

miss-islington added a commit that referenced this pull request Jun 27, 2018
@miss-islington @vstinner
bpo-33929: multiprocessing: fix handle leak on race condition (GH-7921) …
8b1ebcd 
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 vstinner mentioned this pull request Jun 27, 2018
Closed
@vstinner

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

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?

vstinner added a commit that referenced this pull request Jun 27, 2018
@vstinner
Revert "bpo-33929: multiprocessing: fix handle leak on race condition (… …
7dac610 
…GH-7921)"

This reverts commit 8b1ebcd.
@vstinner

vstinner commented Jun 27, 2018

Copy link
Copy Markdown
Member Author

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

vstinner added a commit that referenced this pull request Jun 27, 2018
@vstinner
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
@terryjreedy
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
@terryjreedy
Revert "bpo-33929: multiprocessing: fix handle leak on race condition (… …
f331080 
…GH-7921)"

This reverts commit 8b1ebcd.
@richardsheridan richardsheridan mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou

@applio applio Awaiting requested review from applio

Assignees

@vstinner vstinner

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @pitrou @miss-islington @bedevere-bot @the-knights-who-say-ni