Skip to content

Commit

Permalink
Merge PR #282 by @meaksh
Browse files Browse the repository at this point in the history
This change populates the `Keep-Alive` header for persistent HTTP/1.1
connections. It helps mitigate race conditions when the client still
thinks that the connection is going to persist but the server closes
it on timeout meanwhile.

The effect of introducing this change is that the HTTP client will now
know about the timeout (because it'll be exposed) and won't attept to
reuse the connection when it's timed out but hasn't gotten the FIN
packet on the underlying TCP connection.

Refs:
* https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive
* https://tools.ietf.org/html/rfc2068#section-19.7.1.1
* https://github.com/meaksh/cheroot/blob/107650d/cheroot/server.py#L736
* https://github.com/meaksh/cheroot/blob/107650d/cheroot/server.py#L1352
  • Loading branch information
webknjaz committed Aug 12, 2020
2 parents b6edd3a + 5612915 commit 4f3906f
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 0 deletions.
13 changes: 13 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
.. scm-version-title:: v8.4.3

- :pr:`282`: Fixed a race condition happening when an HTTP
client attempts to reuse a persistent HTTP connection after
it's been discarded on the server in :py:class:`~cheroot.\
server.HTTPRequest` but no TCP FIN packet has been receiced
yet over the wire -- by :user:`meaksh`.

This change populates the ``Keep-Alive`` header exposing
the timeout value for persistent HTTP/1.1 connections which
helps mitigate such race conditions by letting the client
know not to reuse the connection after that time interval.

.. scm-version-title:: v8.4.2

- Fixed a significant performance regression introduced in
Expand Down
6 changes: 6 additions & 0 deletions cheroot/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1175,6 +1175,12 @@ def send_headers(self):
if not self.close_connection:
self.outheaders.append((b'Connection', 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'),
))

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
Expand Down
12 changes: 12 additions & 0 deletions cheroot/test/test_conn.py
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +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,
)

# Remove the keep-alive header again.
status_line, actual_headers, actual_resp_body = test_client.get(
Expand All @@ -396,6 +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 not header_exists('Keep-Alive', actual_headers)

test_client.server_instance.protocol = original_server_protocol

Expand All @@ -422,8 +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 not header_exists('Keep-Alive', actual_headers)

disconnect_errors = (
http_client.BadStatusLine,
Expand Down

0 comments on commit 4f3906f

Please sign in to comment.