From 91cdcc4edbaa31a14a81c6e52853a3f4e948bd50 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:28:25 -0400 Subject: [PATCH 1/9] Make resp.close(force=True) default --- aiohttp/client_reqrep.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 53cdede61c4..30f047876da 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -598,7 +598,10 @@ def start(self, connection, read_until_eof=False): 'Can not load response cookies: %s', exc) return self - def close(self, force=False): + def close(self, force=True): + if not force: + warnings.warn("force parameter should be True", DeprecationWarning, + level=2) if self._closed: return @@ -609,17 +612,9 @@ def close(self, force=False): return if self._connection is not None: - if self.content and not self.content.at_eof(): - force = True - - if force: - self._connection.close() - else: - self._connection.release() - if self._reader is not None: - self._reader.unset_parser() - + self._connection.close() self._connection = None + if self._writer is not None and not self._writer.done(): self._writer.cancel() self._writer = None @@ -631,7 +626,16 @@ def release(self): while chunk is not EOF_MARKER or chunk: chunk = yield from self.content.readany() finally: - self.close() + if self._connection is not None: + assert self.content.at_eof() + self._connection.release() + if self._reader is not None: + self._reader.unset_parser() + self._connection = None + + if self._writer is not None and not self._writer.done(): + self._writer.cancel() + self._writer = None @asyncio.coroutine def wait_for_close(self): @@ -640,7 +644,7 @@ def wait_for_close(self): yield from self._writer finally: self._writer = None - self.close() + yield from self.release() @asyncio.coroutine def read(self, decode=False): @@ -649,10 +653,10 @@ def read(self, decode=False): try: self._content = yield from self.content.read() except: - self.close(True) + self.close() raise else: - self.close() + yield from self.release() data = self._content From a94941e462ae790c7eee89875ac9399c0579565b Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:46:22 -0400 Subject: [PATCH 2/9] Fix one test --- tests/test_client_response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_response.py b/tests/test_client_response.py index a60416bf051..cecdea9fd0e 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -97,7 +97,7 @@ def test_release(self): self.response.close = unittest.mock.Mock() self.loop.run_until_complete(self.response.release()) - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_read_and_close(self): self.response.read = unittest.mock.Mock() From 74d57198f1d071e6fe96017a95a1ab35663ad83f Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:47:37 -0400 Subject: [PATCH 3/9] Fix another test --- tests/test_client_response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_response.py b/tests/test_client_response.py index cecdea9fd0e..1c2ad59dad6 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -87,7 +87,7 @@ def test_read_and_release_connection_with_error(self): self.assertRaises( ValueError, self.loop.run_until_complete, self.response.read()) - self.response.close.assert_called_with(True) + self.response.close.assert_called_with() def test_release(self): fut = asyncio.Future(loop=self.loop) From 354716d2d44ed6b247ac917654a13d49485d29e0 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:50:04 -0400 Subject: [PATCH 4/9] Fix more tests --- aiohttp/client_reqrep.py | 6 +++--- tests/test_client_response.py | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 30f047876da..62e5f47e40a 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -622,12 +622,12 @@ def close(self, force=True): @asyncio.coroutine def release(self): try: - chunk = yield from self.content.readany() - while chunk is not EOF_MARKER or chunk: + if not self.content.at_eof(): chunk = yield from self.content.readany() + while chunk is not EOF_MARKER or chunk: + chunk = yield from self.content.readany() finally: if self._connection is not None: - assert self.content.at_eof() self._connection.release() if self._reader is not None: self._reader.unset_parser() diff --git a/tests/test_client_response.py b/tests/test_client_response.py index 1c2ad59dad6..5aa6cb7f758 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -44,7 +44,6 @@ def test_close(self): self.response._connection = self.connection self.response.close() self.assertIsNone(self.response.connection) - self.assertTrue(self.connection.release.called) self.response.close() self.response.close() From 70f88e19e09608b29619648d8a87fdaedebb7977 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:52:31 -0400 Subject: [PATCH 5/9] Pass next test --- tests/test_client_response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_response.py b/tests/test_client_response.py index 5aa6cb7f758..0aa4933ef25 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -165,7 +165,7 @@ def side_effect(*args, **kwargs): self.loop.run_until_complete(self.response.read()) res = self.loop.run_until_complete(self.response.text()) self.assertEqual(res, '{"тест": "пройден"}') - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_text_after_read(self): def side_effect(*args, **kwargs): From 58e4dbf61492a647aa09aad34fbf0c45f7d034a7 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 21:53:47 -0400 Subject: [PATCH 6/9] Pass yet another test --- tests/test_client_response.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_client_response.py b/tests/test_client_response.py index 0aa4933ef25..a9be94713e2 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -149,7 +149,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete( self.response.text(encoding='cp1251')) self.assertEqual(res, '{"тест": "пройден"}') - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) self.assertFalse(self.response._get_encoding.called) def test_text_detect_encoding(self): @@ -180,7 +180,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete(self.response.text()) self.assertEqual(res, '{"тест": "пройден"}') - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_json(self): def side_effect(*args, **kwargs): From d67b89ceb83b2e383b56555d6993974de7cc2110 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 22:04:12 -0400 Subject: [PATCH 7/9] Make all tests passed --- aiohttp/client_reqrep.py | 7 ++++--- tests/test_client_response.py | 10 +++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 62e5f47e40a..b1a54d8b9a0 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -622,10 +622,11 @@ def close(self, force=True): @asyncio.coroutine def release(self): try: - if not self.content.at_eof(): - chunk = yield from self.content.readany() + content = self.content + if content is not None and not content.at_eof(): + chunk = yield from content.readany() while chunk is not EOF_MARKER or chunk: - chunk = yield from self.content.readany() + chunk = yield from content.readany() finally: if self._connection is not None: self._connection.release() diff --git a/tests/test_client_response.py b/tests/test_client_response.py index a9be94713e2..f467e81a0e7 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -75,7 +75,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete(self.response.read()) self.assertEqual(res, b'payload') - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_read_and_release_connection_with_error(self): content = self.response.content = unittest.mock.Mock() @@ -132,7 +132,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete(self.response.text()) self.assertEqual(res, '{"тест": "пройден"}') - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_text_custom_encoding(self): def side_effect(*args, **kwargs): @@ -195,7 +195,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete(self.response.json()) self.assertEqual(res, {'тест': 'пройден'}) - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_json_custom_loader(self): self.response.headers = { @@ -236,7 +236,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete( self.response.json(encoding='cp1251')) self.assertEqual(res, {'тест': 'пройден'}) - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) self.assertFalse(self.response._get_encoding.called) def test_json_detect_encoding(self): @@ -251,7 +251,7 @@ def side_effect(*args, **kwargs): res = self.loop.run_until_complete(self.response.json()) self.assertEqual(res, {'тест': 'пройден'}) - self.assertTrue(self.response.close.called) + self.assertIsNone(self.response._connection) def test_override_flow_control(self): class MyResponse(ClientResponse): From 3b44e77ea8ae588683a14e7911d9ab1963ea6191 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 31 Aug 2015 22:08:01 -0400 Subject: [PATCH 8/9] Update CHANGES --- CHANGES.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index 3ba9fde2576..d48fdcc73de 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -14,3 +14,7 @@ CHANGES * default headers in ClientSession are now case-insensitive * Make '=' char and 'wss://' schema safe in urls #477 + +* `ClientResponse.close()` forces connection closing by default from now #479 + N.B. Backward incompatible change: was `.close(force=False) + Using `force` parameter for the method is deprecated: use `.release()` instead. From 91f5f051a17682c710e2c5f1f205589966465626 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Tue, 1 Sep 2015 00:26:53 -0400 Subject: [PATCH 9/9] Improve coverage --- aiohttp/client_reqrep.py | 15 +++++++-------- tests/test_client_response.py | 6 ++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index b1a54d8b9a0..0a945f1212f 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -601,7 +601,7 @@ def start(self, connection, read_until_eof=False): def close(self, force=True): if not force: warnings.warn("force parameter should be True", DeprecationWarning, - level=2) + stacklevel=2) if self._closed: return @@ -614,10 +614,7 @@ def close(self, force=True): if self._connection is not None: self._connection.close() self._connection = None - - if self._writer is not None and not self._writer.done(): - self._writer.cancel() - self._writer = None + self._cleanup_writer() @asyncio.coroutine def release(self): @@ -633,10 +630,12 @@ def release(self): if self._reader is not None: self._reader.unset_parser() self._connection = None + self._cleanup_writer() - if self._writer is not None and not self._writer.done(): - self._writer.cancel() - self._writer = None + def _cleanup_writer(self): + if self._writer is not None and not self._writer.done(): + self._writer.cancel() + self._writer = None @asyncio.coroutine def wait_for_close(self): diff --git a/tests/test_client_response.py b/tests/test_client_response.py index f467e81a0e7..4798dcbe222 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -268,3 +268,9 @@ def test_get_encoding_unknown(self, m_chardet): self.response.headers = {'CONTENT-TYPE': 'application/json'} self.assertEqual(self.response._get_encoding(), 'utf-8') + + def test_close_deprecated(self): + self.response._connection = self.connection + with self.assertWarns(DeprecationWarning): + self.response.close(force=False) + self.assertIsNone(self.response._connection)