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.

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

Nagle #283

Merged
merged 1 commit into from Jan 16, 2014

Conversation

Projects
None yet
3 participants
@kevinburke
Contributor

kevinburke commented Nov 18, 2013

No description provided.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Nov 18, 2013

I deleted my remote branch, that closed the old PR. This one is cleaner now

def _new_conn(self):
try:
conn = socket.create_connection(
(self.host,self.port),

This comment has been minimized.

@shazow

shazow Nov 18, 2013

Collaborator

PEP8 please (space after comma).

@@ -104,4 +161,6 @@ def connect(self):
if ssl:
# make a copy for testing.

This comment has been minimized.

@shazow

shazow Nov 18, 2013

Collaborator

Capitalize the first letter...

Also I like this solution, thanks.

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 18, 2013

Looks fairly sane. Is there anything left to do here?

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Nov 18, 2013

Yeah - what do you think about making this configurable with a flag passed to the connectionpool?

}
class HTTPConnection(_HTTPConnection, object):

This comment has been minimized.

@shazow

shazow Nov 18, 2013

Collaborator

TIL this works...

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 18, 2013

Yeah - what do you think about making this configurable with a flag passed to the connectionpool?

I'm -1 to exposing yet another flag to the public interface for now, but perhaps this is a better compromise: What if we add a disable_nagle = True to the connection class, that people can override if they want? Then we can check if self.disable_nagle: ... before disabling it.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Nov 18, 2013

Ok, updated

self.timeout,
)
if self.tcp_nodelay:
conn.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)

This comment has been minimized.

@shazow

shazow Nov 18, 2013

Collaborator

If we're going to call it literally tcp_nodelay, then would it make more sense to just have 0 and 1 values, and skip the if statement with...

conn.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, self.tcp_nodelay)

?

This comment has been minimized.

@kevinburke

kevinburke Nov 18, 2013

Contributor

That's not a bad idea. The advantage of naming it tcp_nodelay is it's familiar and obvious what it does to the people who want the setting. The disadvantage (I think) with that and disable_nagle is the use of a negative in the variable name, which gets doubled if you set tcp_nodelay = False.

This comment has been minimized.

@shazow

shazow Nov 18, 2013

Collaborator

Agreed. Let's always explicitly set it, that way we also get the benefit of consistent behaviour despite the possibility of varying default socket options.

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 18, 2013

Ping @HonzaKral, this is relevant to your interests. :)

@HonzaKral

This comment has been minimized.

HonzaKral commented Nov 18, 2013

Thanks for the ping, and for the code obviously :) It looks good.

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 18, 2013

Disable Nagle's Algorithm on the socket for non-proxies. (Issue #254)

How are we leaving Nagle enabled for proxies? (Am I missing something?)

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Nov 19, 2013

I'm not sure what was meant by that comment. We can only disable Nagle (set socket options) in places where we actually create the socket. I grepped the source code for other references to sock or socket (places where we could set the socket option) and couldn't find anything.

I'm not too familiar with proxies though, it's possible I'm missing something.

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 19, 2013

I think this optimization is harmful for proxies, since the underlying data transmission is less fragmented. At least if you look at other libs like httplib2, they forego disabling nagle in proxies.

Perhaps what we need to do is change ProxyManager so that it overrides PoolManager._new_pool(...), calls the super, but sets the returned constructed pool's tcp_nodelay=0 right before returning it (and add a note there why we do it). Does that make sense?

@HonzaKral Are you aware of any of the details re nagle and proxies?

@HonzaKral

This comment has been minimized.

HonzaKral commented Nov 19, 2013

@shazow no, I am not aware of any differences and from the nature of things I don't understand how it would be harmful. But then again there is a reason why I am not a network programmer ;)

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Nov 20, 2013

@shazow I agree that packets can get a little smaller. I guess as a general note, it's also possible/likely people are implementing proxies outside of Python, for example connecting to HAProxy or Squid running on the same machine.

I'm a little more hesitant to disable this by default... it is useful for my usecase (a time-critical API where we absolutely want packets sent as fast as possible) and @HonzaKral's use case as well, but not sure it is useful in the general case.

Browsing other HTTP clients it seems like they generally leave it off by default. This is by no means an extensive list.

For completeness, it does look like this library sets it.

@shazow

This comment has been minimized.

Collaborator

shazow commented Nov 20, 2013

Also https://code.google.com/p/httplib2/source/browse/python2/httplib2/__init__.py#886 Though the py3 version of httplib2 does not have any such thing.

Thinking of it rationally, since the Nagle algorithm clumps together smaller packets into one send, it makes sense to keep it enabled for proxies, which would have all packets going to the same place, thus would have more benefit from Nagle.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 16, 2014

Sorry... picking this back up again. That's fair. I haven't worked too much with proxies and thought of them as two distinct HTTP hops, kind of like Squid, instead of a single request that is made by way of a middle server.

@shazow

This comment has been minimized.

Collaborator

shazow commented Jan 16, 2014

@kevinburke Thanks for picking this up again, Kevin.

Is this good to be merged on your end?

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 16, 2014

Just need to figure out how to configure the setting for proxies...

Kevin Burke | Twilio
phone: 925.271.7005 | kev.inburke.com

On Wed, Jan 15, 2014 at 5:48 PM, Andrey Petrov notifications@github.comwrote:

@kevinburke https://github.com/kevinburke Thanks for picking this up
again, Kevin.

Is this good to be merged on your end?


Reply to this email directly or view it on GitHubhttps://github.com//pull/283#issuecomment-32435440
.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 16, 2014

Pushed code that I think does what we want; I am worried by the lack of testing so I am adding tests that the socket options are set the way we expect.

@shazow

This comment has been minimized.

Collaborator

shazow commented Jan 16, 2014

Could you git rebase master; git push --force ... to get rid of the extraneous commits and make it mergeable?

I am adding tests that the socket options are set the way we expect.

Excellent, thank you.

Let me know when I should do a final review and merge. :)

Disable Nagle's algorithm for non-proxies
Nagle's algorithm can be useful in some scenarios to limit packet overhead and
bandwidth over the wire. This change allows consumers to disable Nagle's
algorithm by subclassing HTTPConnection in urllib3/connection.py.
@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 16, 2014

Ok, per your comment in the other PR I changed back the names. I also updated some documentation references

shazow added a commit that referenced this pull request Jan 16, 2014

Merge pull request #283 from kevinburke/nagle
Disable Nagle's algorithm for non-proxies

@shazow shazow merged commit 951ea12 into urllib3:master Jan 16, 2014

1 check passed

default The Travis CI build passed
Details
@shazow

This comment has been minimized.

Collaborator

shazow commented Jan 16, 2014

Strange how the PR attributes these commits to me, but oh well. :) Thanks again, Kevin!

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 16, 2014

Yeah I think it's cause I squashed all of my commits onto yours... I'm sure
I could fix it if I knew how to git

On Thursday, January 16, 2014, Andrey Petrov notifications@github.com
wrote:

Strange how the PR attributes these commits to me, but oh well. :) Thanks
again, Kevin!


Reply to this email directly or view it on GitHubhttps://github.com//pull/283#issuecomment-32537418
.

Kevin Burke | Twilio
phone: 925.271.7005 | kev.inburke.com

@shazow

This comment has been minimized.

Collaborator

shazow commented Jan 16, 2014

The squash prolly wasn't necessary, just a plain rebase would have done it.

@HonzaKral

This comment has been minimized.

HonzaKral commented Feb 28, 2014

This is an important change for us, can we maybe get a release with this in it?

@shazow

This comment has been minimized.

Collaborator

shazow commented Feb 28, 2014

@HonzaKral I've been waiting for the Retry PR before pushing v1.8 but I guess there's no real reason to wait. I'll try to push this out in the next few days. Sorry for the delay. Don't hesitate to bug me like this. :)

@shazow

This comment has been minimized.

Collaborator

shazow commented Mar 6, 2014

@HonzaKral Good news, v1.8 is published. :) Enjoy!

@kevinburke kevinburke deleted the kevinburke:nagle branch Aug 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment