Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2009)

#26741: subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by victor.stinner
Modified:
1 year, 6 months ago
Reviewers:
vadmium+py
CC:
AntoinePitrou, haypo, devnull_psf.upfronthosting.co.za, Martin Panter, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/subprocess.py View 2 chunks +8 lines, -1 line 1 comment Download
Lib/test/test_subprocess.py View 10 chunks +33 lines, -28 lines 2 comments Download

Messages

Total messages: 1
Martin Panter
1 year, 6 months ago #1
https://bugs.python.org/review/26741/diff/16954/Lib/subprocess.py
File Lib/subprocess.py (right):

https://bugs.python.org/review/26741/diff/16954/Lib/subprocess.py#newcode1528
Lib/subprocess.py:1528: if pid == self.pid:
Is it possible for this to not be the case, i.e. waitpid() returns a pid we
didn’t ask for?

Also, what is the point of calling _handle_exitstatus()? Isn’t the code about to
raise an exception and throw this info away?

https://bugs.python.org/review/26741/diff/16954/Lib/test/test_subprocess.py
File Lib/test/test_subprocess.py (right):

https://bugs.python.org/review/26741/diff/16954/Lib/test/test_subprocess.py#n...
Lib/test/test_subprocess.py:781: self.addCleanup(p.stdout.close)
Can we drop this cleanup now?

https://bugs.python.org/review/26741/diff/16954/Lib/test/test_subprocess.py#n...
Lib/test/test_subprocess.py:2273: p = None
I’m a bit curious why you changed “del p” to “p = None”. IMO the original is
more explicit.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7