classification
Title: Small fixes around the use of TCP MSS in http.client
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: demian.brecht, martin.panter, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2015-01-22 17:00 by demian.brecht, last changed 2015-01-23 16:03 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue23302.patch demian.brecht, 2015-01-22 17:02 review
issue23302_1.patch demian.brecht, 2015-01-23 06:47 review
Messages (5)
msg234500 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-01-22 17:00
There are a couple of small issues with the determination of whether or not a request can fit in a single TCP/IP packet in http.client.

1. The MSS is hardcoded
2. The TCP data size is calculated as only the message body. This is incorrect as the size of the HTTP headers should also be accounted for.

I suggest two changes be made to fix this:

1. The MSS is retrieved (on platforms that support it) using getsockopt(socket.IPPROTO_TCP, socket.TCP_MAXSEG). If the platform doesn't support it (i.e. Windows), it should default to the currently hardcoded value. This does add a requirement for a connection to be established prior to this calculation (currently the connection is only established after).
2. The HTTP headers are added to the full payload size calculation when compared against the connection's MSS value.

This is still a best guess based on minimal TCP/IP header sizes, but if options or extension headers are used, the packet may still be split at the lower levels. This is fine because what we're trying to avoid is multiple send()s where the payload is less than the MSS.
msg234524 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2015-01-22 23:02
Or we should acknowledge that this is overkill, and take the same approach as all major web browser: disable the Nagle algorithm.
For a protocol like http which is transaction oriented it's probably the best thing to do.
msg234542 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-01-23 04:33
I'm not opposed to that either. The only downside really (at least as far as I'm aware) is the potential substantial influx of packets should an iterable comprised of small chunks of data be passed in as the body, although I would consider that quite a strange edge case for HTTP requests.


I'll put together another patch that disables nagle by default and see if anyone has a contrary opinion.
msg234545 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-01-23 06:47
I've attached a new patch disabling Nagle by default, but doing so in connect() as to allow users to override it if they really want to. I've also removed the use of mss in HTTPConnection. This is a backwards incompatible change in two ways:

1. Removing mss as a public attribute of the HTTPConnection. This /could/ be left as a property that pulls the TCP_MAXSEG option once a connection has been established, but I think it's better to just remove it altogether and have the users call into <HTTPConnection>.sock.getsockopt() instead.
2. If someone is expecting to be able to rely on Nagle by default, although I think this is rather unlikely for HTTP.

That said, I do agree that this is a simpler, more portable choice to solve the problem.
msg234561 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-23 16:03
New changeset 42971b769c0b by Benjamin Peterson in branch 'default':
http.client: disable Nagle's algorithm (closes #23302)
https://hg.python.org/cpython/rev/42971b769c0b
History
Date User Action Args
2015-01-23 16:03:23python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg234561

resolution: fixed
stage: resolved
2015-01-23 06:47:06demian.brechtsetfiles: + issue23302_1.patch
type: behavior
messages: + msg234545
2015-01-23 04:33:41demian.brechtsetmessages: + msg234542
2015-01-22 23:02:45neologixsetnosy: + pitrou, neologix
messages: + msg234524
2015-01-22 20:54:12martin.pantersetnosy: + martin.panter
2015-01-22 17:02:39demian.brechtsetfiles: + issue23302.patch
keywords: + patch
components: + Library (Lib)
versions: + Python 3.5
2015-01-22 17:00:35demian.brechtcreate