Skip to content

Conversation

@mrocklin
Copy link
Member

@mrocklin mrocklin commented May 1, 2022

Fixes #6255

Previously this would cause a RuntimeError during shutdown.

test incoming

@mrocklin
Copy link
Member Author

mrocklin commented May 1, 2022

Added test. I think that we used to have such a test. Maybe it was removed? I'd encourage folks to take a look in case it seems bad in some way.

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2022

Unit Test Results

       16 files  ±  0         16 suites  ±0   7h 59m 2s ⏱️ + 4m 12s
  2 745 tests +  2    2 662 ✔️ +  2       80 💤 ±0  3 ±0 
21 861 runs  +16  20 810 ✔️ +13  1 044 💤 ±0  7 +3 

For more details on these failures, see this check.

Results for commit 40a21a6. ± Comparison against base commit 223c815.

Comment on lines +1604 to +1607
try:
await to_thread(_close)
except RuntimeError: # Are we shutting down the process?
_close() # Just run it directly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference to be explicit here but I'm fine either way. The following works as well, at least for the test

Suggested change
try:
await to_thread(_close)
except RuntimeError: # Are we shutting down the process?
_close() # Just run it directly
from distributed.utils import is_python_shutting_down
if is_python_shutting_down():
# Scheduling new futures during shutdown is not allowed
_close()
else:
await to_thread(_close)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to ask for forgiveness rather than permission here. I don't have enough confidence about Python shutting down to know all of the stages involved. Honestly I'd also be ok just except Exception here. The worst thing that can happen is that we call close again. I'd rather take the sledgehammer than the scalpel approach here.

@fjetter fjetter merged commit 02a4212 into dask:main May 2, 2022
@mrocklin mrocklin deleted the quiet-close branch May 2, 2022 15:30
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.

"Bug" introduced in 2022.04.2 requiring explicit cluster close

2 participants