From 933220d2c48b37d9070776d3f899d587930a838a Mon Sep 17 00:00:00 2001
From: Adam Horacek <adam.horacek@gmail.com>
Date: Thu, 14 Jan 2021 12:14:18 +0100
Subject: [PATCH] Eliminate side-effects from the `ClientResponse.ok` property

This change makes it so accessing `ClientResponse.ok` only does the status
code check.

Prior to this commit, it'd call `ClientResponse.raise_for_status()` which
in turn, closed the underlying TCP session whenever the status was 400 or
higher making it effectively impossible to keep working with the response,
including reading the HTTP response payload.

PR #5404 by @adamko147

Fixes #5403

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
---
 CHANGES/5403.bugfix           |  1 +
 CONTRIBUTORS.txt              |  1 +
 aiohttp/client_reqrep.py      |  8 ++------
 tests/test_client_response.py | 21 +++++++++++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)
 create mode 100644 CHANGES/5403.bugfix

diff --git a/CHANGES/5403.bugfix b/CHANGES/5403.bugfix
new file mode 100644
index 00000000000..40cc5a22294
--- /dev/null
+++ b/CHANGES/5403.bugfix
@@ -0,0 +1 @@
+Stop automatically releasing the ``ClientResponse`` object on calls to the ``ok`` property for the failed requests.
diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index b1278f52325..48c122174c4 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -3,6 +3,7 @@
 A. Jesse Jiryu Davis
 Adam Bannister
 Adam Cooper
+Adam Horacek
 Adam Mills
 Adrian Krupa
 Adrián Chaves
diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py
index 97b2ce25f00..801bc6dcda7 100644
--- a/aiohttp/client_reqrep.py
+++ b/aiohttp/client_reqrep.py
@@ -923,14 +923,10 @@ def ok(self) -> bool:
         This is **not** a check for ``200 OK`` but a check that the response
         status is under 400.
         """
-        try:
-            self.raise_for_status()
-        except ClientResponseError:
-            return False
-        return True
+        return 400 > self.status
 
     def raise_for_status(self) -> None:
-        if 400 <= self.status:
+        if not self.ok:
             # reason should always be not None for a started response
             assert self.reason is not None
             self.release()
diff --git a/tests/test_client_response.py b/tests/test_client_response.py
index 9b81e3ea6f2..9f3c4990085 100644
--- a/tests/test_client_response.py
+++ b/tests/test_client_response.py
@@ -1243,3 +1243,24 @@ def test_response_links_empty(loop: Any, session: Any) -> None:
     )
     response._headers = CIMultiDict()
     assert response.links == {}
+
+
+def test_response_not_closed_after_get_ok(mocker) -> None:
+    response = ClientResponse(
+        "get",
+        URL("http://del-cl-resp.org"),
+        request_info=mock.Mock(),
+        writer=mock.Mock(),
+        continue100=None,
+        timer=TimerNoop(),
+        traces=[],
+        loop=mock.Mock(),
+        session=mock.Mock(),
+    )
+    response.status = 400
+    response.reason = "Bad Request"
+    response._closed = False
+    spy = mocker.spy(response, "raise_for_status")
+    assert not response.ok
+    assert not response.closed
+    assert spy.call_count == 0