bpo-35721: Close socket pair if Popen in _UnixSubprocessTransport fails #11553
Conversation
| universal_newlines=False, bufsize=bufsize, **kwargs) | ||
| if stdin_w is not None: | ||
| self._proc.stdin = open(stdin_w.detach(), 'wb', buffering=bufsize) | ||
| stdin.close() |
There was a problem hiding this comment.
Please move the line before open() call.
It saves 1 open file descriptor.
Not a very big deal but why not?
There was a problem hiding this comment.
I'm not sure I even understand this code :) What's it for? Why does monkeypatching self._proc.stdin work?
There was a problem hiding this comment.
@1st1 please read a code comment at the begin of the method.
AIX requires to make stdin a socket pair (I love all Unix flavors and their differences).
proc.stdin should be a writable file-like object.
open() creates a file object from write-fd of created socket pair.
It exists in asyncio starting from very old times.
There was a problem hiding this comment.
@asvetlov: Good point. I suppose since open(stdin_w.detach(), ...) cannot fail I should have kept the order.
There was a problem hiding this comment.
@1st1 please read a code comment at the begin of the method.
Yeah, that's what I did first. The comment doesn't explain why we pass the read end of the pipe to Popen initially (I see that we then monkey-patch it to the write end).
It exists in asyncio starting from very old times.
I've no problem with the code or this PR, but I wish that the comment is a bit more elaborate.
There was a problem hiding this comment.
@1st1 you know, I'm bad in comment writing, sorry.
Would you propose exact text?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
niklasf
force-pushed
the
socket-pair-leak-35721
branch
from
3b43fd0 to
e6a1cf3
Compare
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
niklasf
force-pushed
the
socket-pair-leak-35721
branch
from
e6a1cf3 to
e19f742
Compare
|
Is there anything I can do to help this move along? |
|
Sorry, I can't merge this PR. Reason: |
|
Thanks @niklasf for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
…ls (pythonGH-11553) This slightly expands an existing test case `test_popen_error` to trigger a `ResourceWarning` and fixes it. https://bugs.python.org/issue35721 (cherry picked from commit 9932fd9) Co-authored-by: Niklas Fiekas <[email protected]>
|
GH-13439 is a backport of this pull request to the 3.7 branch. |
|
Let's land it. |
…ls (GH-11553) This slightly expands an existing test case `test_popen_error` to trigger a `ResourceWarning` and fixes it. https://bugs.python.org/issue35721 (cherry picked from commit 9932fd9) Co-authored-by: Niklas Fiekas <[email protected]>
|
Thanks! |
This slightly expands an existing test case
test_popen_errorto trigger aResourceWarningand fixes it.https://bugs.python.org/issue35721