-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix connector #2567
Fix connector #2567
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.3 #2567 +/- ##
==========================================
- Coverage 97.35% 97.34% -0.01%
==========================================
Files 39 39
Lines 8253 8260 +7
Branches 1437 1439 +2
==========================================
+ Hits 8035 8041 +6
Misses 96 96
- Partials 122 123 +1
Continue to review full report at Codecov.
|
@@ -343,6 +343,28 @@ def test_release_close(loop): | |||
assert proto.close.called | |||
|
|||
|
|||
def test__drop_acquire_per_host1(loop): |
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.
@asvetlov is this double__underscore
notation intentional?
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. It is a test for _drop_acquire_per_host
method.
Why the clients that are waiting for an available connection hang in case of the previous one had an error? I cant see this bad behavior in the current code. Could you please point me to the right path? Also, I don't get the point of the |
self._waiters was not cleaned up, https://github.com/aio-libs/aiohttp/blob/2.3/aiohttp/connector.py#L357 said that That's why I need review the logic for |
self_acquired is enough.
Dropped a check for |
Oks, got it. LGTM can be merged once all checks passes |
IMHO this kind of bug is enough important to have a bugfix release for the 2.X series |
@pfreixes will publish 2.3.4 just after merging |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs. |
Prevent hang on connection acquiring after connection failure