Issue #16833: In http.client.HTTPConnection, do not concatenate the request headers and body when the payload exceeds 16 KB, since it can consume more memory for no benefit.

Patch by Benno Leslie.

4 files changed, 42 insertions, 2 deletions

diff --git a/Lib/http/client.py b/Lib/http/client.py
index 5e5b3a3..7db79b3 100644
--- a/Lib/http/client.py
+++ b/Lib/http/client.py
@@ -719,6 +719,14 @@ class HTTPConnection:
     default_port = HTTP_PORT
     auto_open = 1
     debuglevel = 0
+    # TCP Maximum Segment Size (MSS) is determined by the TCP stack on
+    # a per-connection basis.  There is no simple and efficient
+    # platform independent mechanism for determining the MSS, so
+    # instead a reasonable estimate is chosen.  The getsockopt()
+    # interface using the TCP_MAXSEG parameter may be a suitable
+    # approach on some operating systems. A value of 16KiB is chosen
+    # as a reasonable estimate of the maximum MSS.
+    mss = 16384
 
     def __init__(self, host, port=None, strict=_strict_sentinel,
                  timeout=socket._GLOBAL_DEFAULT_TIMEOUT, source_address=None):
@@ -886,8 +894,11 @@ class HTTPConnection:
         del self._buffer[:]
         # If msg and message_body are sent in a single send() call,
         # it will avoid performance problems caused by the interaction
-        # between delayed ack and the Nagle algorithm.
-        if isinstance(message_body, bytes):
+        # between delayed ack and the Nagle algorithm. However,
+        # there is no performance gain if the message is larger
+        # than MSS (and there is a memory penalty for the message
+        # copy).
+        if isinstance(message_body, bytes) and len(message_body) < self.mss:
             msg += message_body
             message_body = None
         self.send(msg)
diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py
index 9c53e9c..e2d644d 100644
--- a/Lib/test/test_httplib.py
+++ b/Lib/test/test_httplib.py
@@ -27,8 +27,10 @@ class FakeSocket:
         self.text = text
         self.fileclass = fileclass
         self.data = b''
+        self.sendall_calls = 0
 
     def sendall(self, data):
+        self.sendall_calls += 1
         self.data += data
 
     def makefile(self, mode, bufsize=None):
@@ -558,6 +560,28 @@ class BasicTest(TestCase):
         self.assertEqual(resp.read(), b'')
         self.assertTrue(resp.isclosed())
 
+    def test_delayed_ack_opt(self):
+        # Test that Nagle/delayed_ack optimistaion works correctly.
+
+        # For small payloads, it should coalesce the body with
+        # headers, resulting in a single sendall() call
+        conn = client.HTTPConnection('example.com')
+        sock = FakeSocket(None)
+        conn.sock = sock
+        body = b'x' * (conn.mss - 1)
+        conn.request('POST', '/', body)
+        self.assertEqual(sock.sendall_calls, 1)
+
+        # For large payloads, it should send the headers and
+        # then the body, resulting in more than one sendall()
+        # call
+        conn = client.HTTPConnection('example.com')
+        sock = FakeSocket(None)
+        conn.sock = sock
+        body = b'x' * conn.mss
+        conn.request('POST', '/', body)
+        self.assertGreater(sock.sendall_calls, 1)
+
 class OfflineTest(TestCase):
     def test_responses(self):
         self.assertEqual(client.responses[client.NOT_FOUND], "Not Found")
diff --git a/Misc/ACKS b/Misc/ACKS
index 1f0b1e0..9a7b3a7 100644
--- a/Misc/ACKS
+++ b/Misc/ACKS
@@ -705,6 +705,7 @@ Luke Kenneth Casson Leighton
 Tshepang Lekhonkhobe
 Marc-André Lemburg
 John Lenton
+Benno Leslie
 Christopher Tur Lesniewski-Laas
 Alain Leufroy
 Mark Levinson
diff --git a/Misc/NEWS b/Misc/NEWS
index 161d4fd..eaa8cfc 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -199,6 +199,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #16833: In http.client.HTTPConnection, do not concatenate the request
+  headers and body when the payload exceeds 16 KB, since it can consume more
+  memory for no benefit.  Patch by Benno Leslie.
+
 - Issue #16541: tk_setPalette() now works with keyword arguments.
 
 - Issue #16820: In configparser, `parser.popitem()` no longer raises ValueError.