From 211e7cf9f60ce7b46416dc1fc5d3bb2075053788 Mon Sep 17 00:00:00 2001 From: aggieNick02 Date: Fri, 27 Sep 2024 14:53:58 -0500 Subject: [PATCH 1/2] Fix cases in Session.request method where an exception could lead to exiting the method without the created socket being owned by a response or closed. This would then cause future calls to get a socket with the same parameters to fail because one already existed and hadn't been closed. The PR https://github.com/adafruit/Adafruit_CircuitPython_Requests/pull/72 fixed this issue for one case where an exception could be raised. This generalizes that in an attempt to fix it in other cases that could still be hit. --- adafruit_requests.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/adafruit_requests.py b/adafruit_requests.py index f0c9e65..427b9e1 100644 --- a/adafruit_requests.py +++ b/adafruit_requests.py @@ -622,6 +622,10 @@ def request( # noqa: PLR0912,PLR0913,PLR0915 Too many branches,Too many argumen # We may fail to send the request if the socket we got is closed already. So, try a second # time in that case. + # Note that the loop below actually tries a second time in other failure cases too, + # namely timeout and no data from socket. This was not covered in the stated intent of the + # commit that introduced the loop, but removing the retry from those cases could prove + # problematic to callers that now depend on that resiliency. retry_count = 0 last_exc = None while retry_count < 2: @@ -643,17 +647,23 @@ def request( # noqa: PLR0912,PLR0913,PLR0915 Too many branches,Too many argumen if ok: # Read the H of "HTTP/1.1" to make sure the socket is alive. send can appear to work # even when the socket is closed. - if hasattr(socket, "recv"): - result = socket.recv(1) - else: - result = bytearray(1) - try: + # Both recv/recv_into can raise OSError; when that happens, we need to call + # _connection_manager.close_socket(socket) or future calls to _connection_manager.get_socket() + # for the same parameter set will fail + try: + if hasattr(socket, "recv"): + result = socket.recv(1) + else: + result = bytearray(1) socket.recv_into(result) - except OSError: - pass - if result == b"H": - # Things seem to be ok so break with socket set. - break + if result == b"H": + # Things seem to be ok so break with socket set. + break + else: + raise RuntimeError("no data from socket") + except (OSError, RuntimeError) as exc: + last_exc = exc + pass self._connection_manager.close_socket(socket) socket = None From 4c9372bb32598eaf1427ba31f63b459b866fb946 Mon Sep 17 00:00:00 2001 From: aggieNick02 Date: Fri, 27 Sep 2024 14:59:58 -0500 Subject: [PATCH 2/2] fix line length in comment --- adafruit_requests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/adafruit_requests.py b/adafruit_requests.py index 427b9e1..5bcf0e4 100644 --- a/adafruit_requests.py +++ b/adafruit_requests.py @@ -648,8 +648,8 @@ def request( # noqa: PLR0912,PLR0913,PLR0915 Too many branches,Too many argumen # Read the H of "HTTP/1.1" to make sure the socket is alive. send can appear to work # even when the socket is closed. # Both recv/recv_into can raise OSError; when that happens, we need to call - # _connection_manager.close_socket(socket) or future calls to _connection_manager.get_socket() - # for the same parameter set will fail + # _connection_manager.close_socket(socket) or future calls to + # _connection_manager.get_socket() for the same parameter set will fail try: if hasattr(socket, "recv"): result = socket.recv(1)