test_nanny_worker_port_range hangs on Windows #5956
Conversation
crusaderky
commented
Mar 17, 2022
•
crusaderky
commented
- Closes test_nanny_worker_port_range hangs on Windows #5925
- Prevent popen'ed subprocess from getting stuck because the stdout/stderr pipe is full
- Dump popen stdout/stderr on gen_test/gen_cluster timeout
- Overhaul test_dask_worker.py
Unit Test Results 12 files + 4 12 suites +4 7h 16m 40s ⏱️ + 4h 7m 35s For more details on these failures, see this check. Results for commit 1b3e6c3. ± Comparison against base commit 2d3fddc. ♻️ This comment has been updated with latest results. |
crusaderky
force-pushed
the
test_nanny_worker_port_range
branch
from
8ad80e8
to
6a9abbe
Compare
crusaderky
force-pushed
the
test_nanny_worker_port_range
branch
from
6a9abbe
to
68fbbcc
Compare
|
To clarify: this PR does not make the tests with |
| print("\n\nPrint from stdout\n=================\n") | ||
| print(out.decode()) | ||
| if dump_stdout: | ||
| print("\n" + "-" * 27 + " Subprocess stdout/stderr" + "-" * 27) |
There was a problem hiding this comment.
| print("\n" + "-" * 27 + " Subprocess stdout/stderr" + "-" * 27) | |
| print("\n" + "-" * 27 + " Subprocess stdout" + "-" * 27) |
Are you not printing err because kwargs["stderr"] = subprocess.STDOUT means it's already been printed?
There was a problem hiding this comment.
Yes, they are merged together because I was afraid of the subprocess getting stuck on stdout while a test is reading from stderr and vice versa.
There was a problem hiding this comment.
Oh I see yes, stderr is redirected into stdout, which is then captured.
| @@ -152,7 +119,8 @@ async def test_no_reconnect(c, s, nanny): | |||
| "--no-reconnect", | |||
| nanny, | |||
| "--no-dashboard", | |||
| ] | |||
| ], | |||
| flush_output=False, | |||
There was a problem hiding this comment.
Why not flush_output=True here and remove the await to_thread(worker.communicate, timeout=5)? Maybe there's a subtle difference in timing I'm missing.
There was a problem hiding this comment.
This and another test were abusing Proc.communicate to just wait for the subprocess to finish. I changed them now.
|
Following up here @crusaderky . Thank you for doing this PR. I'm curious, a lot of the tests that I've run into that fail unexpectedly use popen. Do you have thoughts on any additional cleanup here that might make sense? I don't have any ideas in particular, this comment is just a shot in the dark. |
|
(I found this PR through git blame) |
