Popen move from wait to communicate to avoid deadlock #5302
Conversation
|
I do not understand why the second last run failed, or why the last changes fixed it... Thus said, I believe this is good enough for review now. |
|
Switching from
|
|
I added the test and made the change more invasive, leading to clearer code. I documented the two spots with the most wiredness... |
|
@pekkaklarck can we up-prioritise this PR? It solves 2 issues, the deadlock and the timeout issue. I tried to balance the intrusiveness of the patch with code quality, but maybe some more aggressive usage of "communicate" can simplify this code for the cost of an invasive changeset. Maybe this is welcome in the long run, maybe for the 8.0 target? |
|
I tested the code and it fixes both #4173 and #5345 which is awesome. The code is rather complicated, though, and there are some style issues like accessing private attributes of the
Because the fix is based on your solution, you'd be acknowledged in the release notes in all these cases. |
|
|
||
| _take_stdout = (process.stdout and (not process.stdout.closed)) | ||
| _take_stderr = (process.stderr and (not process.stderr.closed)) | ||
|
|
There was a problem hiding this comment.
Style:
- We don't generally use empty line inside methods. If methods get so long that they need to be grouped by empty lines, we prefer introducing helper methods. In this particular case empty lines could just be removed.
- We don't use
_prefix with local variables. - All parentheses are unnecessary.
| # This is needed to handle stdin closes identically to how it was when the | ||
| # wait function was used. | ||
| _stdin = process.stdin | ||
| process.stdin = process.stdin if (process.stdin and not process.stdin.closed) else io.StringIO("") |
There was a problem hiding this comment.
It's true that Popen.communicate() doesn't like stdin to be a closed PIPE, but there can also be problems if stdout and stderr are closed PIPEs. How they are all handled is pretty inconsistent and I submitted a bug about that:
python/cpython#131064
To be consistent with our old Popen.wait() based code, we need to handle all streams. Closed streams can be replaced with None instead io.StringIO(""), though.
| # this is to allow the process to be aborted when on windows | ||
| # an exception is used to mange the test timeout situation | ||
| (_stdout, _stderr,) = process.communicate(timeout=0.1) | ||
| break |
There was a problem hiding this comment.
This isn't related to #4173 anymore and should rather be submitted as a separate PR. It also needs a separate test.
In my opinion try/except would make the code easier to understand that contextlib.suppress.
|
|
||
| for f in [process.stdout, process.stderr] + result._custom_streams: | ||
| with contextlib.suppress(AttributeError): | ||
| f.close() |
There was a problem hiding this comment.
Accessing _private members of the result object like the above code does is not ok. Making result.stdout property settable is a better solution.
|
I looked at this again, and noticed that the change broke a case where only *** Test Cases ***
Example
${p}= Start process python -V stderr=DEVNULL
Call method ${p.stdout} close
${r}= Wait for process
Log to console ${r.stdout}I have fixed the problem locally and added a test for it. I've also fixed other issues I mentioned in the review, and it would probably be easiest if I committed my changes and closed this PR. I'll wait until tomorrow with that in case you want to fix the issues yourself. Regardless how we proceed, big thanks for the PR. It proved that the deadlock with PIPEs can be avoided. |
|
Sorry, I was unavailable the last few days. Sure, go ahead and commit + push if you already have the changes available. Most of the things mentioned here I have no strong opinion about, except for the 'close stdout' issue, which should be fixed. |
The code is based on PR #5302 by @franzhaas, but contains some cleanup, doc updates, and fix for handling closed stdout/stderr PIPEs. Fixes #4173.
|
I committed my changes in f2f2f99. Notice that those changes don't contain the fix for timeouts (#5345) but I'll fix that separately. Huge thanks for your work @franzhaas! |
The fix is based on the code in PR #5302 by @franzhaas. Fixes #5345. Now that `Popen.communicate()` gets a timeout, its implementation always gets to a code path where stdtout/stderr being a closed PIPE doesn't matter and we only need to care about stdin. As the result content written to PIPEs that are later closed is now available.
see here