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

@gpshead gpshead 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
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
Copy link
Member Author

gpshead 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
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
that belongs in its own PR if at all.
it wasn't actually testing what it described as it never checked
the grandchild process explicitly.  redo this better in a later PR.
@brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Jul 5, 2019
@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
@miss-islington
Copy link
Contributor

miss-islington 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 pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019
…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
Copy link

bedevere-bot commented Sep 11, 2019

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 11, 2019

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019
…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
…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
…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>
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
…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
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants