Allow the lifetime of the Connection thread to be tied to an event loop #112
Conversation
|
The lint error with Python 3.7 and 3.8 is unrelated to this PR but can be fixed with [DESIGN]
max-parents=9 |
| @@ -47,13 +47,15 @@ def __init__( | |||
| connector: Callable[[], sqlite3.Connection], | |||
| iter_chunk_size: int, | |||
| loop: Optional[asyncio.AbstractEventLoop] = None, | |||
| parent_loop: Optional[asyncio.AbstractEventLoop] = None, | |||
There was a problem hiding this comment.
Is there a reason or benefit to explicitly passing the parent loop (something that was explicitly deprecated in previous versions), rather than just using get_loop to get the active loop when the connection object is made? Using get_loop instead will prevent errors if the non-active loop is passed in, and also provide more consistent behavior in the run() method.
I think I would also prefer tracking this as self._loop for brevity.
There was a problem hiding this comment.
I added it as an explicit parameter to preserve existing behavior and because get_loop may not immediately run, like if self._tx is never filled with any futures, and it would pick the first-seen event loop in the case of multiple loops, which may not be desired.
There was a problem hiding this comment.
If get_loop is called within the _connect method, then there's never a chance that it would get the wrong event loop. I'd overall prefer to have consistent behavior with minimal parameters than have more parameters that result in a higher number of distinct codepaths.
| @@ -116,6 +118,19 @@ def set_exception(fut, e): | |||
|
|
|||
| get_loop(future).call_soon_threadsafe(set_exception, future, e) | |||
|
|
|||
| # Clean up within this thread only if the parent event loop exits ungracefully | |||
| if not self._running or self._connection is None or self._parent_loop is None: | |||
There was a problem hiding this comment.
This isn't obvious as to when it will return or run the cleanup below. Is there a way to always try cleaning up?
| with self.assertRaises(ValueError): | ||
| db.in_transaction |
There was a problem hiding this comment.
Maybe validate db._running also/instead?
|
Agreed. I'm still doing some more testing (specifically preventing IPython lockups when using aiosqlite within an interactive session) but maybe it's better to keep track of every observed event loop via Alternatively, maybe it's better to make the thread daemonic and possibly clean up when the thread is killed (if that is possible)? Or use a second daemon thread to signal the main thread to exit? Or The "parent event loop" thing was just my original idea, I don't particularly care about the implementation details as long as the database is gracefully closed and the interpreter isn't stuck waiting for this thread when it exits. |
Description
In projects where aiosqlite is used just as an async replacement for the
sqlite3module, it's not always possible to refactor downstream codebases to use async context managers and gracefully close aiosqlite. This results in undesirable behavior like Ctrl+C not immediately working, failing integration tests locking up test runners (how I found out GitHub Actions can run for six hours 😄), and the user eventually mashing Ctrl+C to kill the process anyways.For the common use case with only a single event loop, tying the lifetime of the
Connectionobject to this event loop with an optional keyword argument will allow aiosqlite to gracefully close the database connection without breaking any existing code. To increase responsiveness, you could even lower the queue polling timeout from0.1to0.01.Related issues: