Josh Rosenberg <[email protected]> added the comment:
Sounds like the solution you'd want here is to just change each if check in
_communicate, so instead of:
if self.stdout:
selector.register(self.stdout, selectors.EVENT_READ)
if self.stderr:
selector.register(self.stderr, selectors.EVENT_READ)
it does:
if self.stdout and not self.stdout.closed:
selector.register(self.stdout, selectors.EVENT_READ)
if self.stderr and not self.stderr.closed:
selector.register(self.stderr, selectors.EVENT_READ)
The `if self.stdin and input:` would also have to change. Right now it's buggy
in a related, but far more complex way. Specifically if you call it with input
the first time:
1. If some of the input is sent but not all, and the second time you call
communicate you rely on the (undocumented, but necessary for consistency) input
caching and don't pass input at all, it won't register the stdin handle for
read (and in fact, will explicitly close the stdin handle), and the remaining
cached data won't be sent. If you try to pass some other non-empty input, it
just ignores it and sends whatever remains in the cache (and fails out as in
the stdout/stderr case if the data in the cache was sent completely before the
timeout).
2. If all of the input was sent on the first call, you *must* pass input=None,
or you'll die trying to register self.stdin with the selector
The fix for this would be to either:
1. Follow the pattern for self.stdout/stderr (adding "and not
self.stdin.closed"), and explicitly document that repeated calls to communicate
must pass the exact same input each time (and optionally validate this in the
_save_input function, which as of right now just ignores the input if a cache
already exists); if input is passed the first time, incompletely transmitted,
and not passed the second time, the code will error as in the OP's case, but it
will have violated the documented requirements (ideally the error would be a
little more clear though)
or
2. Change the code so populating the cache (if not already populated) is the
first step, and replace all subsequent references to input with references to
self._input (for setup tests, also checking if self._input_offset >=
len(self._input), so it doesn't register for notifications on self.stdin if all
the input has been sent), so it becomes legal to pass input=None on a second
call and rely on the first call to communicate caching it. It would still
ignore new input values on the subsequent calls, but at least it would behave
in a sane way (not closing sys.stdin despite having unsent cached data, then
producing a confusing error that is several steps removed from the actual
problem)
Either way, the caching behavior for input should be properly documented; we
clearly specify that output is preserved after a timeout and retrying
communicate ("If the process does not terminate after timeout seconds, a
TimeoutExpired exception will be raised. Catching this exception and retrying
communication will not lose any output."), but we don't say anything about
input, and right now, the behavior is the somewhat odd and hard to express:
"Retrying a call to communicate when the original call was passed
non-None/non-empty input requires subsequent call(s) to pass non-None,
non-empty input. The input on said subsequent calls is otherwise ignored; only
the unsent remainder of the original input is sent. Also, it will just fail
completely if you pass non-empty input and it turns out the original input was
sent completely on the previous call, in which case you *must* call it with
input=None."
It might also be worth changing the selectors module to raise a more obvious
exception when register is passed a closed file-like object, but given it only
requires non-integer fileobjs to have a .fileno() method, adding a requirement
for a "closed" attribute/property could break other code.
----------
nosy: +josh.r
stage: -> needs patch
_______________________________________
Python tracker <[email protected]>
<https://bugs.python.org/issue35182>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
