Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
demianbrecht mannequin opened this issue Jan 22, 2015 · 5 comments
Closed

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

demianbrecht mannequin opened this issue Jan 22, 2015 · 5 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@demianbrecht
Copy link
Mannequin

demianbrecht mannequin commented Jan 22, 2015

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
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jan 22, 2015

    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.

    @demianbrecht demianbrecht mannequin added the stdlib Python modules in the Lib dir label Jan 22, 2015
    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jan 22, 2015

    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
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jan 23, 2015

    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
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jan 23, 2015

    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.

    @demianbrecht demianbrecht mannequin added the type-bug An unexpected behavior, bug, or error label Jan 23, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 23, 2015

    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

    @python-dev python-dev mannequin closed this as completed Jan 23, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    0 participants