Small fixes around the use of TCP MSS in http.client

[demianbrecht]

BPO	23302
Nosy	@pitrou, @vadmium, @demianbrecht
Files	issue23302.patch issue23302_1.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-01-23.16:03:23.257>
created_at = <Date 2015-01-22.17:00:35.436>
labels = ['type-bug', 'library']
title = 'Small fixes around the use of TCP MSS in http.client'
updated_at = <Date 2015-01-23.16:03:23.254>
user = 'https://github.com/demianbrecht'

bugs.python.org fields:

activity = <Date 2015-01-23.16:03:23.254>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2015-01-23.16:03:23.257>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2015-01-22.17:00:35.436>
creator = 'demian.brecht'
dependencies = []
files = ['37822', '37826']
hgrepos = []
issue_num = 23302
keywords = ['patch']
message_count = 5.0
messages = ['234500', '234524', '234542', '234545', '234561']
nosy_count = 5.0
nosy_names = ['pitrou', 'neologix', 'python-dev', 'martin.panter', 'demian.brecht']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23302'
versions = ['Python 3.5']

[demianbrecht]

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.

[neologix]

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.

[demianbrecht]

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.

[demianbrecht]

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.

[python-dev]

New changeset 42971b769c0b by Benjamin Peterson in branch 'default':
http.client: disable Nagle's algorithm (closes bpo-23302)
https://hg.python.org/cpython/rev/42971b769c0b