Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unblock event loop while waiting for ThreadpoolExecutor to shut down #6091

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Apr 8, 2022

This reinstates #5883
which was reverted in #5961 / #5932

I could confirm the flakyness of test_missing_data_errant_worker after this change and am reasonably certain this is caused by #5910 which causes a closing worker to be restarted such that, even after Worker.close is done, the worker still appears to be partially up.

The only reason I can see why this change promotes this behaviour is that if we no longer block the event loop while the threadpool is closing, this opens a much larger window for incoming requests to come in and being processed while close is running.

Closes #6239

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

Unit Test Results

       16 files  ±0         16 suites  ±0   7h 25m 8s ⏱️ + 13m 29s
  2 741 tests  - 1    2 656 ✔️  - 4       81 💤 ±0  4 +3 
21 829 runs  +8  20 777 ✔️ +4  1 048 💤 +2  4 +2 

For more details on these failures, see this check.

Results for commit 37d9277. ± Comparison against base commit b837003.

♻️ This comment has been updated with latest results.

@fjetter fjetter force-pushed the reinstate_5883 branch 2 times, most recently from 5a903b9 to 64aba9c Compare April 28, 2022 13:46
@fjetter fjetter marked this pull request as ready for review April 28, 2022 13:46
@mrocklin
Copy link
Member

Thoughts on isolating out the unblock commit from the other one? It looks like this PR is based off of #5910 , which seems a bit more complex.

@mrocklin
Copy link
Member

I'm fine with this PR. The increased testing failures are concerning. I'm guessing that it's just noise, but we should both take a look at them so that we feel the pain of them in our brains :)

@mrocklin
Copy link
Member

This one seems maybe interesting: #6247

@mrocklin mrocklin merged commit 70e1fca into dask:main Apr 29, 2022
@fjetter
Copy link
Member Author

fjetter commented Apr 29, 2022

As I described in the above comment, with this change, #5910 becomes more important. However, it seems to come together finally (apart from merge conflicts)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't block event loop in Worker.close waiting for executor to shut down
2 participants