Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-37424: Avoid a hang in subprocess.run timeout output capture #14490

Merged
merged 7 commits into from Sep 11, 2019

Conversation

@gpshead
Copy link
Member

commented Jul 1, 2019

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.

https://bugs.python.org/issue37424

gpshead added 2 commits Jun 30, 2019
bpo-37424: Have subprocess use killpg for some timeout kills.
When shell=True the user is often launching a process tree, if we
kill only the shell and it hasn't set itself up as a pgrp leader,
the grandchild processes still run and may have our output handles
open.  Leading us to wait forever when we should be timing out.

This Adds a fractional timeout on the cleanup communicate phase
_and_ will use killpg on the child process group _if_ it is
different from our own as it should be when start_new_session=True
was used.
@gpshead

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

If an API change is made to make the default behavior easier for people in the future, that part won't get backported to 3.7. 3.8 only.

@gpshead gpshead changed the title bpo-issue37424: Avoid a hang in subprocess.run timeout output capture bpo-37424: Avoid a hang in subprocess.run timeout output capture Jul 1, 2019

@gpshead gpshead requested review from giampaolo and vstinner Jul 1, 2019

Lib/subprocess.py Outdated Show resolved Hide resolved
Lib/subprocess.py Show resolved Hide resolved
Simplify this a lot, do not use magic process groups.
Do not have a post-timeout sub-timeout for output collection, we've already
got all of that output within POSIX's _communicate() method as it is.
Lib/subprocess.py Outdated Show resolved Hide resolved
gpshead added 3 commits Jul 4, 2019
Remove the magic shell pgrp detection killpg logic.
that belongs in its own PR if at all.
Remove session pgrp logic related test.
it wasn't actually testing what it described as it never checked
the grandchild process explicitly.  redo this better in a later PR.

@gpshead gpshead self-assigned this Aug 22, 2019

@gpshead gpshead requested a review from Yhg1s Sep 10, 2019

Lib/subprocess.py Outdated Show resolved Hide resolved
Lib/test/test_subprocess.py Show resolved Hide resolved

@Yhg1s Yhg1s merged commit 580d278 into python:master Sep 11, 2019

5 checks passed

Azure Pipelines PR #20190910.130 succeeded
Details
bedevere/issue-number Issue number 37424 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

commented Sep 11, 2019

Thanks @gpshead for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒🤖

miss-islington added a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019
bpo-37424: Avoid a hang in subprocess.run timeout output capture (pyt…
…honGH-14490)

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.
(cherry picked from commit 580d278)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 11, 2019

GH-15897 is a backport of this pull request to the 3.8 branch.

@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 11, 2019

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019
bpo-37424: Avoid a hang in subprocess.run timeout output capture (pyt…
…honGH-14490)

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.
(cherry picked from commit 580d278)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Sep 11, 2019
bpo-37424: Avoid a hang in subprocess.run timeout output capture (GH-…
…14490)

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.
(cherry picked from commit 580d278)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Sep 11, 2019
bpo-37424: Avoid a hang in subprocess.run timeout output capture (GH-…
…14490)

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.
(cherry picked from commit 580d278)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
vrpolakatcisco added a commit to vrpolakatcisco/cpython that referenced this pull request Sep 12, 2019
bpo-37424: Avoid a hang in subprocess.run timeout output capture (pyt…
…honGH-14490)

Fixes a possible hang when using a timeout on subprocess.run() while
capturing output. If the child process spawned its own children or otherwise
connected its stdout or stderr handles with another process, we could hang
after the timeout was reached and our child was killed when attempting to read
final output from the pipes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.