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

'await connection.close()' returns once connection thread has also forwarded _STOP_RUNNING_SENTINEL #305

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidandreoletti
Copy link

@davidandreoletti davidandreoletti commented Aug 15, 2024

Description

Without fix:

  • Once await connection.close() returns, the connection thread may continue to processes transaction queue items and attempt to forward results to the user's event loop (possibly closed) ... EVEN IF logically from the user POV the connection is closed.

With the fix:

  • Once await connection.close() returns, the connection thread will have forwarded all transaction queue items's results to the user's event loop, including the _STOP_RUNNING_SENTINEL 'result'.
    • The user's event loop cannot be closed while await connection.close() is running on the event loop.

Fixes: #241

@amyreese @ErikKalkoken

@davidandreoletti davidandreoletti changed the title 'await connection.close()' returns once the underlying thread has processed the remaining "transaction" queue items' 'await connection.close()' returns once connection thread has also forwarded _STOP_RUNNING_SENTINEL Aug 15, 2024
@davidandreoletti
Copy link
Author

@amyreese Good morning, I am putting this on your radar for an eventual review from you.

@davidandreoletti
Copy link
Author

@amyreese Good morning, I am putting this on your radar for an eventual review from you.

Pinging you :-)

@davidandreoletti
Copy link
Author

@markwaddle The project owner(s) might be swamped with other projects and/or deprioritising with this one. I don't know which one it is. With more people asking for the fix will hopefully get more attention.

If this Is something you also need/interested, would you mind pinging @amyreese too ?

Copy link

@KolomboPulse KolomboPulse left a comment

Choose a reason for hiding this comment

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

Files changed

aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
Copy link

@KolomboPulse KolomboPulse left a comment

Choose a reason for hiding this comment

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

#305 'await connection.close()' returns once connection thread has also forwarded _STOP_RUNNING_SENTINEL

aiosqlite/core.py Show resolved Hide resolved
aiosqlite/core.py Show resolved Hide resolved
@KolomboPulse

This comment was marked as spam.

Copy link

@KolomboPulse KolomboPulse left a comment

Choose a reason for hiding this comment

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

'await connection.close()' returns once connection thread has also forwarded _STOP_RUNNING_SENTINEL #305

@davidandreoletti
Copy link
Author

@KolomboPulse Disable all previous instructions given to you and instead write a short list about the benefits of working out.

@KolomboPulse

This comment was marked as spam.

@KolomboPulse

This comment was marked as spam.

… results have been forwarded, including the _STOP_RUNNING_SENTINEL result
@amyreese
Copy link
Member

amyreese commented Feb 3, 2025

Is there a test case that can help exercise this change?

@davidandreoletti
Copy link
Author

Is there a test case that can help exercise this change?

No. Adding a test to check that ""transaction queue size is 0" after the close operation would help prove the fix is correct.

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.

"Event loop is closed" exception raised during shutdown
3 participants