This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author josh.r
Recipients and800, josh.r
Date 2018-11-07.20:04:49
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1541621089.36.0.788709270274.issue35182@psf.upfronthosting.co.za>
In-reply-to
Content
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.
History
Date User Action Args
2018-11-07 20:04:49josh.rsetrecipients: + josh.r, and800
2018-11-07 20:04:49josh.rsetmessageid: <1541621089.36.0.788709270274.issue35182@psf.upfronthosting.co.za>
2018-11-07 20:04:49josh.rlinkissue35182 messages
2018-11-07 20:04:49josh.rcreate