Skip to content

narrow Subprocess.__init__ Popen except to real Popen failures#3640

Open
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/process-popen-bare-except-narrow
Open

narrow Subprocess.__init__ Popen except to real Popen failures#3640
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/process-popen-bare-except-narrow

Conversation

@HrachShah

Copy link
Copy Markdown

narrow Subprocess.init Popen except to real Popen failures

The bare 'except:' wrapped subprocess.Popen() and a per-init pipe FD
cleanup loop. The narrow set covers the actual failures of Popen():
OSError (FileNotFoundError when the executable isn't found,
PermissionError when it isn't executable, OSError for other fork/exec
failures), ValueError (embedded null byte in executable path),
TypeError (unknown kwarg), and subprocess.SubprocessError (parent of
all subprocess-raised errors added in Python 3.3; covers the cases where
Popen detects an invalid argument combination internally).

The bare except was also catching BaseException including KeyboardInterrupt
and SystemExit — a Ctrl-C during the Popen call now propagates
immediately to the asyncio task that owns the Subprocess instead of
being swallowed and re-raised after the FD cleanup loop.

The bare 'except:' wrapped subprocess.Popen() and a per-init pipe FD
cleanup loop. The narrow set covers the actual failures of Popen():
OSError (FileNotFoundError when the executable isn't found,
PermissionError when it isn't executable, OSError for other fork/exec
failures), ValueError (embedded null byte in executable path),
TypeError (unknown kwarg), and subprocess.SubprocessError (parent of
all subprocess-raised errors added in Python 3.3; covers the cases where
Popen detects an invalid argument combination internally).

The bare except was also catching BaseException including KeyboardInterrupt
and SystemExit — a Ctrl-C during the Popen call now propagates
immediately to the asyncio task that owns the Subprocess instead of
being swallowed and re-raised after the FD cleanup loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant