Initial work on Keep-Alive

[drewmiller]

An initial attempt at adding Connection: Keep-Alive pooling to SimpleAsyncHTTPClient per #324.

In order to prevent duplication of the url-parsing step, I converted the current process to a class method that is called and compared against idle connections if the reuse_connections attribute is set. If a new connection is required, the "_ConnectionBase" namedtuple is passed as a parameter to the private _HTTPConnection class so that re-parsing is not necessary.

Please let me know what we can do to improve the testing here.

[bdarnell]

Sorry for the delay; this is pretty awesome. It looks like it's in pretty good shape overall. The biggest issue I see on a first pass is the cache key: we need to exclude parsed from _ConnectionBase, or else we will only be able to reuse connections if the url is an exact match. On the other hand, we may need to include some other fields from the request in the cache key: allow_ipv6 and all of the ssl-related options can affect whether it is safe to reuse connections (basically anything that is used in the arguments to TCPClient.connect should be part of the cache key). I think it would also be acceptable to say that any requests with non-default values for any of these parameters would simply have connection reuse disabled.

As for testing, we at least need to verify that connections are reused for non-identical urls, and that they are not when host, port, or scheme doesn't match (use hostname_mapping for the host mismatch tests).

[drewmiller]

Thanks, Ben--very helpful. Apologies for the delay getting back to you on these. We're pushing out a significant update this week. Once we've taken care of that, I'll incorporate your comments.

Including the parsed value in the key was a silly mistake.

As an anecdote, our primitive tests (using ab with 100 concurrent connections) indicate a 100% improvement in response times over the Curl implementation when using a reusable Simple client connection to interact with an Elasticsearch back end. Hopefully others will find this useful in the future as well.

[mqingyn]

I found a problem，After many times requests，I got a exception：
StreamClosedError: Stream is closed

and when there are different parameters on the url,connection can not be reused. @drewmiller

[joshuahlang]

Any idea when this might be merged/released?

[drewmiller]

is curl out of the equation for your application? i sort of abandoned this effort when i realized the curl implementation reused connections. Otherwise, it's not too far off.

Sent from my iPhone

On May 17, 2016, at 7:33 PM, joshuahlang [email protected] wrote:

Any idea when this might be merged/released?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[sonicisthebest]

How do you get the curl version to reuse connections? Even when I pass "Connection: keep-alive" as a header on the fetch the remote side is receiving "Connection: close" from somewhere...

[bdarnell]

The application should never touch the Connection header; let the HTTP framework handle that. Curl should reuse connections automatically whenever possible.

[sonicisthebest]

Even when I don't send any headers at all, I'm still seeing "Connection: close" at the remote side:

AsyncHTTPClient.configure('tornado.curl_httpclient.CurlAsyncHTTPClient')
http = AsyncHTTPClient()
http.fetch('url etc...', callback=...)

Any ideas?

[bdarnell]

The server is not obligated to let you reuse your connection; it can always choose to send Connection: close. However, the vast majority of servers do support connection reuse, and for me it just works without any special configuration besides using the curl http client:

$ python -m tornado.simple_httpclient --print-headers=true --print-body=false http://python.org|grep Connection:
Connection: close
$ python -m tornado.curl_httpclient --print-headers=true --print-body=false http://python.org|grep Connection:
Connection: keep-alive

[sonicisthebest]

Thanks. Solved it, the connection was routed through a little grunt proxy for dev purposes before it reached the tornado backend. It was that proxy that was adding the "Connection: close" and causing the issue.

[bdarnell]

Closing this in favor of the updated version in #1622.