close shared clients when loop closes #579
Conversation
requires monkeypatching asyncio.EventLoop.close to run cleanup functions not sure what other recourse we have for long-running tasks which should cleanup eventually
|
If we duplicate JupyterHub's task cleanup into our test fixtures, I think I can make this work without patching. |
There was a problem hiding this comment.
Beautiful! I think this is relatively little intrusive as it just appends some logic while otherwise preserving all logic.
And I understand it as this only becomes relevant when the event loop shuts down - and that should only happen as part of the entire application starts to shut down - right?
Non-critical comments made - this LGTM as a solution to #578.
no patches required, but does assume that tasks are cancelled when the loop exits. JupyterHub does this, so it's a reasonable assumption for us, but not in general in asyncio.
|
I pushed another commit that avoids any monkeypatching, but instead relies on cleanup of pending tasks, which JupyterHub does in jupyterhub/jupyterhub#2519. |
… must be async KubeSpawner constructor calls shared_client, which calls asyncio.get_running_loop. Could switch to get_event_loop, but that's deprecated. There is no longer a standard API for getting a 'current' but not running loop.
minrk
force-pushed
the
close-shared-client
branch
from
def4cbb
to
8109e91
Compare
|
The one test failure is a real one and a race condition - the reflector tasks themselves may not be canceled before the close-client is canceled, meaning their requests may fail due to a closed session. Need to think about that a bit. We may need to explicitly shut down the reflectors somehow in the right order (garbage collection ensured the right order, but the gc |
when tasks are cancelled, they should stop, not keep retrying removes unnecessary threading Event now that reflectors are in the same thread
minrk
force-pushed
the
close-shared-client
branch
from
c398986
to
3a6074d
Compare
|
@minrk this looks good to me, are you happy with the changes as they are at this point? I'll re-run the test a few times to try catch a potential missed race condition and merge if you are. Can you update the PR description to reflect the current state of how you went about things? |
|
I think the CancelledError handling will actually solve the races, but I can't be 100% sure. If tests are reliable, let's go ahead. |
|
Updated description. |
|
Wieeee @minrk amazing work! I appreciate seeing this complexity reduction in the project and I love learning from reviewing your work! ❤️ 🎉 I ran the test suite five times or so, not a single failure showed up :) |
|
I'm not sure how to label things. This was a bug, but it wasn't released, so, should it be marked as a bug for the changelog that is fixed? On the other hand, should it be labelled a bug to know that it solved a bug in case someone used the default branch version or a developer wanted insight about the purpose of this change? Yikes... Well, I'm going for maintenance now. |
closes #578
Closes
api_clientby running a coroutine that will be cancelled when the event loop finishes holdingwith client.api_client. JupyterHub cancels all tasks prior to closing the loop, so this should be okay, but it isn't rigorous for all uses of asyncio.