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 2 commits 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 ?

@davidandreoletti
Copy link
Author

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

… 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

davidandreoletti commented Feb 4, 2025

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

@amyreese
Yes, now there is 5668138. The test checks the transaction queue size is 0 after the close operation.

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
2 participants