close_session: clean up loop handling #606
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martindurant
reviewed
Feb 8, 2024
- Previously we were attemting syncronous cleanup on the current thread's event loop (even if it wasn't active, having been just created above). Instead, retain our `loop` parameter and use it to perform synchronous cleanup.
pgcamus
force-pushed
the
pg/close_session
branch
from
8af314d to
03c2079
Compare
martindurant
reviewed
Feb 9, 2024
gcsfs/core.py
Outdated
Comment on lines
338
to
346
| loop = asyncio.get_event_loop() | ||
| loop.create_task(session.close()) | ||
| current_loop = asyncio.get_event_loop() | ||
| current_loop.create_task(session.close()) |
There was a problem hiding this comment.
So the difference is, that loop in sync() below is still the original. I don't see how this makes any difference, because the only way to avoid the first return is if get_event_loop() fails, and so no assign happens. Do you find there could be a RuntimeError in create_task() ?
There was a problem hiding this comment.
Yeah. From what I can tell
- The GC weakref callback runs in the main thread.
- The main thread has no event loop, so when we call
get_event_loop()it fails because it creates a new one which has never been started. create_task()then fails because it refuses to create a task on a closed loop, andBaseEventLoop._check_closed()raises the exception.
With this fix, we don't see the exception anymore, but we do see the following, which is related but I don't think is caused by this change.
RuntimeWarning: coroutine 'ClientSession.close' was never awaited
martindurant
reviewed
Feb 9, 2024
Co-authored-by: Martin Durant <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were potentially running cleanup on a different
event loop, even if the session's associated event loop was
active. This manifested in RuntimeError instances being raised
when synchronous cleanup was attempted. Change cleanup to
always use the session's associated event loop and add some
logging when things have gone wrong.