From 30e6b613b37976e42921f8ee1b78a47ddd4edb6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Wed, 6 May 2020 14:04:48 +0100 Subject: [PATCH 1/8] Avoid race condition on persistent HTTP connections Add a HTTP "Keep-Alive" header with "timeout" on the HTTP response to avoid a race condition on persistent HTTP connections when the HTTP client reuses a connection after the "socket.timeout" exception triggered on the HTTPServer but before the FIN packet is produced. When this happens, the client gets a "connection reset by peer" after writting the request. This commit makes a HTTP client to know about this "Keep-Alive" idle timeout by exposing it on the HTTP "Keep-Alive" response header, so the connection won't be reused if it was "idle" for that "timeout" after the last request response. --- cheroot/server.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cheroot/server.py b/cheroot/server.py index 70623cdfe4..a2ce44338f 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1175,6 +1175,11 @@ def send_headers(self): if not self.close_connection: self.outheaders.append((b'Connection', b'Keep-Alive')) + self.outheaders.append(( + b'Keep-Alive', + "timeout={}".format(self.server.timeout).encode('ISO-8859-1'), + )) + if (not self.close_connection) and (not self.chunked_read): # Read any remaining request body data on the socket. # "If an origin server receives a request that does not include an From 257db0127a65033ec8910c6aebc923828c699d2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Fri, 15 May 2020 09:41:07 +0100 Subject: [PATCH 2/8] Fix lint issues --- cheroot/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cheroot/server.py b/cheroot/server.py index a2ce44338f..452cbb4c41 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1177,7 +1177,7 @@ def send_headers(self): self.outheaders.append(( b'Keep-Alive', - "timeout={}".format(self.server.timeout).encode('ISO-8859-1'), + 'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'), )) if (not self.close_connection) and (not self.chunked_read): From a0705d9f89cf69b6c617de97b8e83c73b6b4c795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Mon, 3 Aug 2020 11:39:20 +0100 Subject: [PATCH 3/8] Check for Keep-Alive timeout header on test_conn --- cheroot/test/test_conn.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py index 0672d1e785..1b1fcd2135 100644 --- a/cheroot/test/test_conn.py +++ b/cheroot/test/test_conn.py @@ -385,6 +385,7 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert header_has_value('Connection', 'Keep-Alive', actual_headers) + assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) # Remove the keep-alive header again. status_line, actual_headers, actual_resp_body = test_client.get( @@ -396,6 +397,7 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert not header_exists('Connection', actual_headers) + assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) test_client.server_instance.protocol = original_server_protocol @@ -424,6 +426,7 @@ def request(conn, keepalive=True): assert header_has_value('Connection', 'Keep-Alive', actual_headers) else: assert not header_exists('Connection', actual_headers) + assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) disconnect_errors = ( http_client.BadStatusLine, From b9bbc30b932b116b685e60243b3398ed2f64321b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Mon, 3 Aug 2020 11:42:53 +0100 Subject: [PATCH 4/8] Fix pylint issues --- cheroot/test/test_conn.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py index 1b1fcd2135..8f14851775 100644 --- a/cheroot/test/test_conn.py +++ b/cheroot/test/test_conn.py @@ -385,7 +385,9 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert header_has_value('Connection', 'Keep-Alive', actual_headers) - assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) + assert header_has_value('Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers) # Remove the keep-alive header again. status_line, actual_headers, actual_resp_body = test_client.get( @@ -397,7 +399,9 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert not header_exists('Connection', actual_headers) - assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) + assert header_has_value('Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers) test_client.server_instance.protocol = original_server_protocol @@ -426,7 +430,9 @@ def request(conn, keepalive=True): assert header_has_value('Connection', 'Keep-Alive', actual_headers) else: assert not header_exists('Connection', actual_headers) - assert header_has_value('Keep-Alive', 'timeout={}'.format(test_client.server_instance.timeout), actual_headers) + assert header_has_value('Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers) disconnect_errors = ( http_client.BadStatusLine, From e9ebf0a4280e5cd114ae24531203ec0cc844b6ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Mon, 3 Aug 2020 11:48:46 +0100 Subject: [PATCH 5/8] Adjust lines according to pre-commit suggestions --- cheroot/test/test_conn.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py index 8f14851775..1e45db5ad7 100644 --- a/cheroot/test/test_conn.py +++ b/cheroot/test/test_conn.py @@ -385,9 +385,11 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert header_has_value('Connection', 'Keep-Alive', actual_headers) - assert header_has_value('Keep-Alive', - 'timeout={}'.format(test_client.server_instance.timeout), - actual_headers) + assert header_has_value( + 'Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers, + ) # Remove the keep-alive header again. status_line, actual_headers, actual_resp_body = test_client.get( @@ -399,9 +401,11 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert not header_exists('Connection', actual_headers) - assert header_has_value('Keep-Alive', - 'timeout={}'.format(test_client.server_instance.timeout), - actual_headers) + assert header_has_value( + 'Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers, + ) test_client.server_instance.protocol = original_server_protocol @@ -430,9 +434,11 @@ def request(conn, keepalive=True): assert header_has_value('Connection', 'Keep-Alive', actual_headers) else: assert not header_exists('Connection', actual_headers) - assert header_has_value('Keep-Alive', - 'timeout={}'.format(test_client.server_instance.timeout), - actual_headers) + assert header_has_value( + 'Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers, + ) disconnect_errors = ( http_client.BadStatusLine, From 3d71786d019b9059c81f9e2c0eeed6b78b35d4ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 11 Aug 2020 13:30:11 +0100 Subject: [PATCH 6/8] Only send Keep-Alive timeout header when Connection is set to Keep-Alive --- cheroot/server.py | 9 +++++---- cheroot/test/test_conn.py | 17 +++++++---------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/cheroot/server.py b/cheroot/server.py index 452cbb4c41..ccb4133d73 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1175,10 +1175,11 @@ def send_headers(self): if not self.close_connection: self.outheaders.append((b'Connection', b'Keep-Alive')) - self.outheaders.append(( - b'Keep-Alive', - 'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'), - )) + if dict(self.outheaders).get(b'Connection', b'') == b'Keep-Alive': + self.outheaders.append(( + b'Keep-Alive', + 'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'), + )) if (not self.close_connection) and (not self.chunked_read): # Read any remaining request body data on the socket. diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py index 1e45db5ad7..94c0685cac 100644 --- a/cheroot/test/test_conn.py +++ b/cheroot/test/test_conn.py @@ -401,11 +401,7 @@ def test_keepalive(test_client, http_server_protocol): assert status_line[4:] == 'OK' assert actual_resp_body == pov.encode() assert not header_exists('Connection', actual_headers) - assert header_has_value( - 'Keep-Alive', - 'timeout={}'.format(test_client.server_instance.timeout), - actual_headers, - ) + assert not header_exists('Keep-Alive', actual_headers) test_client.server_instance.protocol = original_server_protocol @@ -432,13 +428,14 @@ def request(conn, keepalive=True): assert actual_resp_body == pov.encode() if keepalive: assert header_has_value('Connection', 'Keep-Alive', actual_headers) + assert header_has_value( + 'Keep-Alive', + 'timeout={}'.format(test_client.server_instance.timeout), + actual_headers, + ) else: assert not header_exists('Connection', actual_headers) - assert header_has_value( - 'Keep-Alive', - 'timeout={}'.format(test_client.server_instance.timeout), - actual_headers, - ) + assert not header_exists('Keep-Alive', actual_headers) disconnect_errors = ( http_client.BadStatusLine, From 99453e2467f900fcd12b5031e3d86bc73a1dccd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Wed, 12 Aug 2020 10:35:10 +0100 Subject: [PATCH 7/8] Update cheroot/server.py Co-authored-by: Sviatoslav Sydorenko --- cheroot/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cheroot/server.py b/cheroot/server.py index ccb4133d73..f90e7bc9c5 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1178,7 +1178,7 @@ def send_headers(self): if dict(self.outheaders).get(b'Connection', b'') == b'Keep-Alive': self.outheaders.append(( b'Keep-Alive', - 'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'), + u'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'), )) if (not self.close_connection) and (not self.chunked_read): From 561291583bfd37d9099ea3f3eca02666353d3089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Wed, 12 Aug 2020 10:40:10 +0100 Subject: [PATCH 8/8] Improve performance looking into the out headers --- cheroot/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cheroot/server.py b/cheroot/server.py index f90e7bc9c5..d6f89dbcdb 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1175,7 +1175,7 @@ def send_headers(self): if not self.close_connection: self.outheaders.append((b'Connection', b'Keep-Alive')) - if dict(self.outheaders).get(b'Connection', b'') == b'Keep-Alive': + if (b'Connection', b'Keep-Alive') in self.outheaders: self.outheaders.append(( b'Keep-Alive', u'timeout={}'.format(self.server.timeout).encode('ISO-8859-1'),