From 549a0d925822a08e3e5b301cb3c638da943b4073 Mon Sep 17 00:00:00 2001 From: Marco Paolini Date: Sat, 22 Aug 2015 19:54:52 +0200 Subject: [PATCH 1/5] Avoid putting a new connection in the free list 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 --- aiohttp/connector.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 2f97dde11da..b3fda689b0b 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -291,12 +291,6 @@ def connect(self, req): else: transport, proto = yield from self._create_connection(req) - if not self._force_close: - if self._conns.get(key, None) is None: - self._conns[key] = [] - - self._conns[key].append((transport, proto, - self._loop.time())) except asyncio.TimeoutError as exc: raise ClientTimeoutError( 'Connection timeout to host %s:%s ssl:%s' % key) from exc From 28c7da472bf66c8eb5d25f05b11697ab7d0f917e Mon Sep 17 00:00:00 2001 From: Marco Paolini Date: Sat, 22 Aug 2015 23:20:40 +0200 Subject: [PATCH 2/5] Fix cleanup of empty pool keys in connector Issues #253 and #254 implemented a `_conns` key evince logic in the function that actually **adds** items to `_conns` Issue #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. --- aiohttp/connector.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index b3fda689b0b..5aa629a2516 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -303,7 +303,10 @@ def connect(self, req): return conn def _get(self, key): - conns = self._conns.get(key) + try: + conns = self._conns[key] + except KeyError: + return None, None t1 = self._loop.time() while conns: transport, proto, t0 = conns.pop() @@ -313,7 +316,9 @@ def _get(self, key): transport = None else: return transport, proto - + # No more connections for this key. Drop refs to transport and protocol + # that make the key + del self._conns[key] return None, None def _release(self, key, req, transport, protocol, *, should_close=False): @@ -351,15 +356,6 @@ def _release(self, key, req, transport, protocol, *, should_close=False): reader = protocol.reader if should_close or (reader.output and not reader.output.at_eof()): - conns = self._conns.get(key) - if conns is not None and len(conns) >= 0: - # Issue #253: An empty array will eventually be - # removed by cleanup, but it's better to pop straight - # away, because cleanup might not get called (e.g. if - # keepalive is False). - if not acquired: - self._conns.pop(key, None) - transport.close() else: conns = self._conns.get(key) From c99d589e051abd9c05b7b23f89016704b134a12b Mon Sep 17 00:00:00 2001 From: Marco Paolini Date: Sun, 23 Aug 2015 00:41:41 +0200 Subject: [PATCH 3/5] Fix some tests --- tests/test_connector.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 56d09b6379c..f657173862c 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -239,7 +239,7 @@ def test_get_expired(self): tr, proto = unittest.mock.Mock(), unittest.mock.Mock() conn._conns[1] = [(tr, proto, self.loop.time() - 1000)] self.assertEqual(conn._get(1), (None, None)) - self.assertEqual(conn._conns[1], []) + self.assertFalse(conn._conns) conn.close() def test_release(self): @@ -278,8 +278,17 @@ def test_release_close(self): self.assertFalse(conn._conns) self.assertTrue(tr.close.called) - def test_release_pop_empty_conns(self): - # see issue #253 + def test_get_pop_empty_conns(self): + # see issue #473 + conn = aiohttp.BaseConnector(loop=self.loop) + key = ('127.0.0.1', 80, False) + conn._conns[key] = [] + tr, proto = conn._get(key) + self.assertEqual((None, None), (tr, proto)) + self.assertFalse(conn._conns) + + def test_release_close_do_not_add_to_pool(self): + # see issue #473 conn = aiohttp.BaseConnector(loop=self.loop) req = unittest.mock.Mock() resp = unittest.mock.Mock() @@ -288,13 +297,10 @@ def test_release_pop_empty_conns(self): key = ('127.0.0.1', 80, False) - conn._conns[key] = [] - tr, proto = unittest.mock.Mock(), unittest.mock.Mock() conn._acquired[key].append(tr) conn._release(key, req, tr, proto) - self.assertEqual({}, conn._conns) - self.assertTrue(tr.close.called) + self.assertFalse(conn._conns) def test_release_close_do_not_delete_existing_connections(self): key = ('127.0.0.1', 80, False) From 8f53917cf82d3ea02daef2fbcff7febe95b7e049 Mon Sep 17 00:00:00 2001 From: Marco Paolini Date: Sun, 23 Aug 2015 00:54:19 +0200 Subject: [PATCH 4/5] Fix evince of connection from pool on error. 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. --- aiohttp/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/client.py b/aiohttp/client.py index 2b145a4d010..534c0508b89 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -134,7 +134,7 @@ def request(self, method, url, *, try: yield from resp.start(conn, read_until_eof) except: - resp.close() + resp.close(force=True) conn.close() raise except (aiohttp.HttpProcessingError, From d0c26e4b805db54dc549cef98a1e0d65fcaaee9e Mon Sep 17 00:00:00 2001 From: Marco Paolini Date: Sun, 23 Aug 2015 01:13:34 +0200 Subject: [PATCH 5/5] Fix connection pool key dropping One branch in the `_get` logic did not drop the key as expected. --- aiohttp/connector.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 5aa629a2516..40ba208bbe7 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -315,9 +315,11 @@ def _get(self, key): transport.close() transport = None else: + if not conns: + # The very last connection was reclaimed: drop the key + del self._conns[key] return transport, proto - # No more connections for this key. Drop refs to transport and protocol - # that make the key + # No more connections: drop the key del self._conns[key] return None, None