Repository navigation
gh-131064: Handle closed pipes consistently in communicate - #159855
limadog9 wants to merge 1 commit into
Conversation
Sorry, something went wrong.
pekkaklarck
left a comment
There was a problem hiding this comment.
As the submitter of the original issue this looks pretty good to me. I only looked at this on a mobile screen, didn't check are all code paths covered and didn't properly check what the added test does, though.
Sorry, something went wrong.
|
@pekkaklarck Thanks for the review. I’ve updated and squashed the changes:
|
Sorry, something went wrong.
|
@pekkaklarck Agreed, I’ve moved fh.close() inside the if not fh.closed block |
Sorry, something went wrong.
| stderr = stderr[0] if stderr else None | ||
| empty_output = '' if self.text_mode else b'' | ||
| stdout = empty_output.join(stdout) if stdout is not None else None | ||
| stderr = empty_output.join(stderr) if stderr is not None else None |
There was a problem hiding this comment.
It seems to me that these changes aren't needed. At this point stdout and stderr are either None or lists self._stdout_buff or self._stderr_buff, respectively. These lists earlier contained exactly one value, but now they can also be empty. The existing if stdout/if stderr conditions cover both the None case and then empty list case.
I consider the current code rather confusing, though, and would consider changing it to something like this:
stdout = None
stderr = None
if self.stdout:
self.stdout.close()
if self._stdout_buff:
stdout = self._stdout_buff[0]
if self.stderr:
self.stderr.close()
if self._stderr_buff:
stderr = self._stderr_buff[0]
return (stdout, stderr)
Sorry, something went wrong.
Make Popen.communicate() handle already-closed pipes consistently across its optimized path, selector path, and Windows reader threads. Closed output pipes return the same empty bytes or string that the selector path already returned. Sending nonempty input to closed stdin raises ValueError before communication starts; calls without input still succeed.
Add regressions covering stdout and stderr, one or multiple pipes, binary and text modes, and calls with and without timeouts. Check that communication can still finish after rejected input. Set the existing broken-pipe mocks' closed state explicitly so they continue to exercise open pipes.
Fixes #131064.
Validation: