Skip to content

Fix connection pooling#473

Merged
fafhrd91 merged 5 commits intoaio-libs:masterfrom
elastic-coders:fix_connection_polling_take2
Aug 26, 2015
Merged

Fix connection pooling#473
fafhrd91 merged 5 commits intoaio-libs:masterfrom
elastic-coders:fix_connection_polling_take2

Conversation

@mpaolini
Copy link
Contributor

@mpaolini mpaolini commented Aug 22, 2015

Fixes weirdness introduced in #254 and #254 and later in #406 .

Supersedes #471

See commit messages for more details

Marco Paolini added 2 commits August 22, 2015 19:54
Avoid putting a new connection in the free list
549a0d9 
The invariant should be: a transport is either in the free list (_conns)
or in the active list (_acquired).

This fixes a bug that put every newly created connection in both lists
Fix cleanup of empty pool keys in connector
28c7da4 
Issues aio-libs#253 and aio-libs#254 implemented a `_conns` key evince logic in the
function that actually **adds** items to `_conns`

Issue aio-libs#406 tweaked this logic even more, making early and eviction
of reusable items in the pool possible.

Here we put the key eviction logic where it belongs: in the method
that **removes** items from the `_conns` pool.
@kxepal
Copy link
Member

kxepal commented Aug 22, 2015

Nice found! But can we have any test to prove that this wouldn't be broken in future?

Marco Paolini added 2 commits August 23, 2015 00:41
Fix some tests
c99d589
Fix evince of connection from pool on error.
8f53917 
Previously, on error we would call the `.close()` method on the response
object. This actually releases the connection instead of closing it,
thus making it available again in the pool.

By passing the `force=True` arg, we make sure the connection is not put
back in the pool.
@mpaolini
Copy link
Contributor Author

mpaolini commented Aug 22, 2015

I did add a specific test and fixed a few existing bous ones, let's see what the buildbots say

Fix connection pool key dropping
d0c26e4 
One branch in the `_get` logic did not drop the key as expected.
@mpaolini mpaolini mentioned this pull request Aug 22, 2015
Closed
@mpaolini mpaolini changed the title Fix connection polling Fix connection pooling Aug 22, 2015
@mpaolini
Copy link
Contributor Author

mpaolini commented Aug 23, 2015

I am testing this in staging now and solves all the that came out upgrading from 0.14.x to 0.17.2

Traceback (most recent call last):
  File "/usr/local/lib/python3.4/site-packages/aiohttp/client.py", line 135, in request
    yield from resp.start(conn, read_until_eof)
  File "/usr/local/lib/python3.4/site-packages/aiohttp/client_reqrep.py", line 566, in start
    self.message = yield from httpstream.read()
  File "/usr/local/lib/python3.4/site-packages/aiohttp/streams.py", line 505, in read
    result = yield from super().read()
  File "/usr/local/lib/python3.4/site-packages/aiohttp/streams.py", line 365, in read
    yield from self._waiter
  File "/usr/local/lib/python3.4/asyncio/futures.py", line 386, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/local/lib/python3.4/asyncio/tasks.py", line 287, in _wakeup
    value = future.result()
  File "/usr/local/lib/python3.4/asyncio/futures.py", line 275, in result
    raise self._exception
aiohttp.errors.ServerDisconnectedError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  [my code here]
  File "/usr/local/lib/python3.4/site-packages/aiohttp/client.py", line 286, in get
    **kwargs)
  File "/usr/local/lib/python3.4/site-packages/aiohttp/client.py

@tdryer
Copy link

tdryer commented Aug 26, 2015

+1, this fixes certain requests consistently timing out in my application starting with aiohttp 0.17.0.

@fafhrd91
Copy link
Member

fafhrd91 commented Aug 26, 2015

+1, those ServerDisconnectedError exceptions are very annoying

fafhrd91 added a commit that referenced this pull request Aug 26, 2015
@fafhrd91
Merge pull request #473 from elastic-coders/fix_connection_polling_take2
3c3fdfa 
Fix connection pooling
@fafhrd91 fafhrd91 merged commit 3c3fdfa into aio-libs:master Aug 26, 2015
@fafhrd91
Copy link
Member

fafhrd91 commented Aug 26, 2015

Thanks!

i'll test changes and then will do new bug fix release

@mpaolini
Copy link
Contributor Author

mpaolini commented Aug 26, 2015

You are welcome! I've tested this on staging for a week now and it works all right

@asvetlov asvetlov added this to the 0.17.3 milestone Aug 28, 2015
@lock
Copy link

lock bot commented Oct 30, 2019

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.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

0.17.3

Development

Successfully merging this pull request may close these issues.

5 participants

@mpaolini @kxepal @tdryer @fafhrd91 @asvetlov