-
-
Notifications
You must be signed in to change notification settings - Fork 748
Ensure connectionpool does not leave comms if closed mid connect #4951
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
Conversation
|
I retriggered the run to see if the register_backend_entrypoint issue persists |
|
The register_backend_entrypoint issue seems to be unrelated (I believe) but is still persistent, see #4961 |
mrocklin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok to me
| # We might still have tasks haning in the semaphore. This will let them | ||
| # run into an exception and raise a commclosed | ||
| while self._n_connecting: | ||
| await asyncio.sleep(0.005) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, the finally block is triggered when we cancel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's covered in the test, ensured by assert pool._connecting in line 651
|
+1 from me |
Co-authored-by: Matthew Rocklin <mrocklin@gmail.com>
66b9a16 to
14c10af
Compare
jrbourbeau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fjetter! This is in
There is a chance for the
ConnectionPoolto leave connections open even after it was closed.This came up during #4734