twisted.web.client persistent connections

[tdavis]

The current twisted.web.client.HTTPClient does not support persistent connections, but since the current implementation is mostly designed for single-use, I am proposing a manager object that handles the management of connections, both persistent and otherwise. The closest corollary here is in twisted.web2.channel.http.HTTPChannel. This method will ensure backwards compatibility with current web.client implementations and should not break any tests -- it is mainly functionality exposed by the new client API (ticket forthcoming on that). Current proposal allows for the following:

1. Maximum number of connections for all domains, likely based on a Cooperator.

2. Persistent connections per-server.

Item (1) is quite simple and would basically function the same way as if you were to use getPage calls yielded to a Cooperator. Item (2) would automatically manage persistent connections on a per-server basis which would timeout after a given amount of time without further requests. IAW RFC 2616 no more than 2 connections would be maintained per-server for a single client. Here is a simple example:

agent = Agent(persist=True)
# Opens connection to foo.com and makes request
response = yield agent.requestString("http://foo.com/file-to-download.txt")
# ...
# Later, reuse same connection to foo.com
response = yield agent.requestString("http://foo.com/some-page.html")
# ...
# X time passes between request/response, open a new connection here
response = yield agent.requestString("http://foo.com/some-other-page.html")

In this way, handling of persistent connections is completely transparent to the user. The easiest and least-disruptive way I see to do this right now is to refactor HTTPPageGetter (which in time will be replaced by a generic protocol that is way better than having to create sub-classes for "getting" and "downloading" and whatever else) and create a manager for HTTPClientFactorys. Assuming HTTPPageGetter has been refactored to keep it from closing the transport connection after a response (and anywhere else applicable), here is a brief example:

def requestString(self, url):
    scheme, host, port, path = _parse(url)
    if self.persist:
        for r in self.requestFactories:
            if r.scheme == scheme and r.host == host and r.port == port:
                # We already have a connection for this, so reuse it
                r.setURL(url)
                r.protocol.connectionMade()
                return
                
    # Make a new factory
    self.requestFactories.append(_makeGetterFactory(url, HTTPClientFactory))

Obviously this is very simplified (and suggests the connection logic is in Agent which isn't terribly likely), but hopefully it gets the point across. I'm not entirely sure if I'm following procedure here, but I wanted to get a sanity check on the idea before I went about writing tests and implementing it. Thanks all.

[exarkun]

It doesn't seem likely to me that Cooperator will be extremely useful in the implementation here, but I could be wrong.

Otherwise, this looks like a good rough sketch to get started on.

[exarkun]

Whatever is implemented should be based on the HTTP client implementation being developed in #886, now.

[exarkun]

Another thing to consider here is how to tell the agent to clean itself up.

[yasusii]

Here's a patch to and the persistent connection feature to twisted.web.client.Agent.

agent = Agent(reactor, persistent=True)

# Opens connection to foo.com and makes request. After the request is
# done, Agent hold the connection as a protocol instance in its cache.
d = agent.request("GET", "http://foo.com/first.html")

# If the first request is done, the second request reuse same
# connection to foo.com, or if it is not done, the second request
# opens new connection.
d = agent.request("GET", "http://foo.com/second.html")

# No more than Agent.maxConnections(default=2) connections would be
# maintained per-server.
# If both the first and second request are not done, the third request
# will be stored in DeferredSemaphore.waiting.
d = agent.request("GET", "http://foo.com/third.html")

[yasusii]

patch to and the persistent connection feature to twisted.web.client.Agent

[exarkun]

(In [30232]) Branching to 'agent-persistent-connections-3420'

[exarkun]

(In [30233]) Apply webclient.diff refs #3420

[exarkun]

Thanks a lot for working on this!

1. twisted/web/client.py
   1. The new Agent attributes maxConnections, _semaphores, _protocolCache, and persistent should be documented in the class docstring.
   2. As Agent will now support persistent connections, we can reverse the claim about them in its class docstring. :)
   3. There needs to be a way to ask an Agent to disconnect all of its cached connections.
2. twisted/web/_newclient.py
   1. The server response needs to be examined for a "Connection: close" header. It may set one even if the request doesn't include one, indicating that the connection is not persistent. Note that this will turn up in Response.connHeaders, not Response.headers.
   2. The new persistent attribute on HTTP11ClientProtocol doesn't appear to be used (and will not be accurate given the previous point). It can be removed, perhaps.
   3. HTTP11ClientProtocol._finishResponse still unconditionally calls _giveUp, which calls transport.loseConnection(). Presumably this needs to be conditional on whether the transport is actually persistent. :)
3. twisted/web/test/test_webclient.py
   1. There should probably be tests that verify that the protocol cache key is computed correctly. test_persistentRequest only hits one scheme/server, so the code that makes sure the wrong connection isn't re-used won't be exercised.
   2. AgentTests._dummyConnect probably isn't quite good enough yet for tests about connection management (the _giveUp problem above perhaps being indicative of this). It might make sense to introduce another stub that more accurately reflects a real transport. It probably wouldn't hurt to have one test that sets up a real connection, too. It will provide a good sanity check so we can be confident the rest of the tests are exercising everything they need to exercise.
4. It would also be great to expand the web client howto (doc/web/howto/client.xhtml) to cover this functionality.

I've applied the patch to the branch now mentioned in the ticket description, so please make that the base for future patches. Thanks again!

[yasusii]

Thanks for your review. Here's a new patch for the branch. I expanded the web client howto too, but I am not very good at english writing :)

[jknight]

• You seem to have conflated "max outstanding connections to a server" with "max cached persistent connections to a server" when persistent=True. These should be separate values, and if you wish to support a "max outstanding connections to a server" value, it ought to be in effect even when persistent=False. That perhaps doesn't really need to be part of this ticket, though.

Firefox sets these to 15 (network.http.max-connections-per-server) and 6 (network.http.max-persistent-connections-per-server). Firefox also has a global "max connections to any server" value, which defaults to 30 (network.http.max-connections). Note that these values are significantly over the RFC's "2" SHOULD -- that is intentional.

• You need to check the request method before making a request over an already-established connection -- only GET/HEAD are supposed to be sent over a persistent connection, because of the likelihood that you might need to retry, and retrying requests with other methods is not considered kosher.

• You need to check the HTTP version to know whether the connection is persistent: 1.0 doesn't allow persistent connections by default while 1.1 does.

• The client should timeout persistent connections on its own, even if the server doesn't. (firefox defaults to 300 seconds, network.http.keep-alive.timeout).

Here's the code from twisted.web2.channel.http to determine what kind of persistence to support, you need something like this too:

        if self.version >= (1,1):
            if 'close' in connHeaders.getHeader('connection', ()):
                readPersistent = False
            else:
	        readPersistent = PERSIST_PIPELINE
        elif 'keep-alive' in connHeaders.getHeader('connection', ()):
            readPersistent = PERSIST_NO_PIPELINE
        else:
            readPersistent = False

I didn't review the tests or docs.

[glyph]

Since James already covered most of the functional stuff, I'm going to focus on docstrings and tests, as covered by the twisted development and review process pages.

1. The docstring for the new Agent._request method needs to be a lot clearer, or perhaps the docstring for Agent.request needs to be improved. Both request and _request say "Issue a new request", but clearly they do different things. It should be obvious which does what. The underscore which starts Agent._request's method name is what says that users shouldn't call it: the question is, who should call it and why? What is the difference between what it does and what Agent.request does?
2. We doesn't use of assert in new code. If it's an error condition that is at all important, then we should have a real, documented exception with a helpful message. If it is testing a condition that "can't happen", then we should have tests for the cases where it might happen, to make sure that we get correct results and it doesn't. Please remove the assert in _finishResponse and either add an if/raise, or some more tests which make sure that any relevant inputs don't trigger that case.
3. The state attribute really shouldn't be exposed. Before this change, the only interface required by Agent._protocol was the request method. That narrowness was good: let's try to keep it that way. Agent should be able to track the relevant state itself, perhaps by wrapping the Response object up so that it can have an additional observer on the bodyDataFinished callback, or perhaps Response should grow an additional method for this purpose, let's say Response.whenDone() which can be called multiple times to return multiple Deferreds that will all fire when the body has finished being delivered. (If tracking state is really the way that you'd prefer to implement it, then the different state constants really need to be named, documented, and exported as public, so that it's not just comparing against a random string.)
4. maxConnections should really be ...
   1. ... called maxConnectionsPerHostName. I know that's a bit of a mouthful, but in the future we may want an actual maxConnections, i.e., a total number of connections for the Agent as a whole, not just per host. (Also: it's good to be as clear as possible that we're limiting by hostname and not by IP.)
   2. ... passed as an argument to Agent.__init__.
   3. ... defaulted to at least 2. (Why is it 1 by default, when the documentation even notes that the RFC's value is 2?)
5. The convention we use for documenting boolean types is C{bool}. Really, this should be L{bool}, but that's a pydoctor issue: "Boolean", however, means nothing in Python.
6. The new narrative documentation is great to have, thanks for adding that, but it should reference  the actual RFC on persistent connections somewhere very early.
7. The new names in the tests - DelayResource, GetBodyProtocol, and getBody - need docstrings.
8. closeCachedConnections() needs to return a Deferred, so that any tests which want to use persistent connections (or an IService implementation which wants to correctly implement stopService) can know that all the persistent connections are shut down before finishing the test.
9. The patch should include a NEWS file, as described in this section of the review process documentation.
10. The assertions in the tests could be more useful:
    1. The checks in the tests which say assertIsInstance(x, list) and then assertEquals(len(x), 0) are a bit too restrictive, especially given that they're testing private implementation details. It's easier to read and produces more useful failures to simply do assertEquals([], x).
    2. assertTrue(x is not None) should be replaced with assertNotIdentical(x, None)

Thanks again for your contribution. I hope that some other folks will chip in and deal with some of these review feedback points - don't feel like you need to submit one giant patch that addresses them all, dealing with one or two at a time would be great :).

[zaheerm]

fix to leaking protocols

[zaheerm]

I just attached a patch to the branch that fixes the issue where if there are more than 1 connections and maxConnections is set appropriately, when coming out of the semaphore the previous code just popped protocols out of the stack until it found one in the QUIESCENT state. This would cause existing connections to not be reused if it isn't the first protocol that is in the QUIESCENT state.

[mbp]

You need to check the request method before making a request over an already-established connection -- only GET/HEAD are supposed to be sent over a persistent connection, because of the likelihood that you might need to retry, and retrying requests with other methods is not considered kosher.

rfc2616 §8.1.2 say that you should not pipeline non-idempotent operations, because if the pipeline collapses those requests could need to be retried. As well as GET and HEAD, PUT and DELETE are also idempotent, and they can be safely retried. (As a practical matter it may be hard to retry a PUT from within Twisted if the body can't be regenerated.)

But the main point is that the RFC permits sending non-idempotent requests as long as they are not pipelined, ie the response has been fully read and the connection is quiescent. At this point it is just as safe as sending them over a brand new connection. (The server might decide to timeout the connection while the client is sending, but in that case it will know the request was not received.)

[glyph]

Thanks for the patch. I don't think this is actually in review though; the patch is to the branch and could just be applied to the branch by the next person who works on it. In the future though, even such patches against branches should come with tests; otherwise it's not clear that it really fixes anything and the author needs to reverse engineer it.

zaheerm, if you could help address some of the feedback in my earlier review, that would be great.

[exarkun]

It looks like the closeCachedConnections method might have a problem similar to the one described in #3207. Some care about re-entrancy might be merited. Or it might be fine as written now, since loseConnection probably doesn't actually do anything synchronously (but you can't really know that).

Another thing that comes to mind is that if loseConnection doesn't do anything synchronously, then you don't really know when the connections have all been closed. Calling closeCachedConnections should cause them to eventually close, but it's anybody's guess as to when.

[c00w]

Um what needs to be done with this branch before it is merged? I'd be willing to help but I need a little clearer guidelines.

[exarkun]

Read <http://twistedmatrix.com/trac/ticket/3420#comment:15> and address the points it raises.

[c00w]

Fix for MaxConnections and the Boolean issue

[itamarst]

(In [32845]) Branching to 'agent-persistent-connections-3420-2'

[itamar]

Merged forward. No actual substantive issues were addressed, but the stuff in updates.diff is mostly in.

[itamar]

OK, time for another set of eyeballs; probably not quite done, but I've been working on this too haphazardly to be a good self-reviewer. Some questions in additional to the usual:

1. Is it worth having an end-to-end test with real connections?

2. Should I ensure redirects reuse same connection, or just open a new ticket for that?

Test run I just started: buildbot.twistedmatrix.com/boxes-supported?branch=/branches/agent-persistent-connections-3420-2

[itamar]

Oh, here's at least one issue. If you have an HTTPS client with different context factory than previous one you probably shouldn't reuse connections... but that's hidden behind the endpoint abstraction. Thoughts?

[itamar]

Replying to itamar:

Oh, here's at least one issue. If you have an HTTPS client with different context factory than previous one you probably shouldn't reuse connections... but that's hidden behind the endpoint abstraction. Thoughts?

Actually, not an issue. Context factory is per Agent, and each agent has its own pool, so we're good.

[exarkun]

1. Is it worth having an end-to-end test with real connections?

One of the often advertised strengths of Twisted is the ability to deterministically unit test network code by avoiding using the actual network. So, uh, no, let's not do this. If there's something we can't be confident is well tested without using real connections, let's add some better testing tools.

[exarkun]

Here's just the start of a review:

1. The branch is too big - almost 2000 lines of diff. Please aim for changes about 1/3rd this size.
2. The doc builder has a new error
3. it doesn't show up as an error, but I don't think @itype is anything in epytext
4. @since markers only belong on public APIs.
   1. Please remove the new one on _HTTPConnectionPool
   2. _AgentMixin seems to have morphed into _HTTP11ClientFactory, but kept the original @since marker - this is exactly why private APIs shouldn't have these. Please remove it from _HTTP11ClientFactory.
5. DelayResource(1)? DelayResource(2)? There's got to be a better way. :/
6. don't use assertEquals, use assertEqual
7. The documentation talks about maxConnections. I don't see anything about this in the implementation.

[itamar]

I think we're dealing with mismatch between natural unit of work and natural unit of review. So, I propose an experiment. If it goes well we might want to use it for all complex patches (number of lines in patch doesn't necessarily map to difficulty of review).

Review for this ticket can be broken up into multiple stages. Until stage 1 has been approved, stage 2 review should not be done, etc..

1. Overall design: Does the public API make sense, do we support the minimal necessary set of features, and can we support likely future features? Are the documentation change and news file sufficient and correct?
2. Detailed review of twisted.web.client changes, along with its tests.
3. Detailed review of twisted.web._newclient changes, along with its tests.

To begin with, I would like a review of stage 1. Once that is done, please remove review keyword and reassign to me. I will fix as necessary, and depending on what the feedback was resubmit for additional stage 1 review, or continue on to request review of stage 2.

[exarkun]

Overall design: Does the public API make sense, do we support the minimal necessary set of features, and can we support likely future features? Are the documentation change and news file sufficient and correct?

The main API change in this branch looks like the new persistent argument to Agent. The default of not making Agent use persistent connections is good, because of backwards compatibility. The fact that the resulting pool ends up private is unfortunate, though, because it means there's no way to ask it to shut down. Perhaps a better API would be to have Agent accept as an initializer argument an actual pool object to use. For backwards compatibility, a default (eg None) could direct it to use a non-persistent "pool".

Then application code can pass in a pool, if it wants, configured however it wants. This means an interface between Agent and the pool needs to be specified (rather than the current implicit, private interface). That interface probably shouldn't be exactly like _connectAndRequest. The Agent should be responsible for adding the Host header, for example. It seems like a method that returns a Deferred that fires with a connected _HTTP11ClientProtocol instance. The other change to the pool interface is that the cleanup method should return a Deferred that fires when the connections have all actually closed.

The scope of the documentation looks good, but the details will need to be updated of course. And I don't see a news fragment. :)

[itamar]

Another issue: connections are returned to pool *after* final callback on response body handler, so two requests in a row (e.g. in documentation example!) won't actually reuse same connection.

[itamar]

Note to self: proxies (http or transparent) should probably be implemented with custom connection pools, not in the Agent.

[itamar]

Actually, exarkun's suggestion for the connection pool's public API has allowed me to make a generic pool implementation that will hopefully work for both regular and proxy agents, so disregard previous comment.

[jknight]

In reply to itamar's IRC comment "I can't find any evidence's of foom's claim that only GET and HEAD should be used for persistent connections"

The reason GET/HEAD are special is because of this, from  http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-17 (also in the http 1.1 non-bis rfc I think but I didn't look)

6.1.5. Retrying Requests
   Senders can close the transport connection at any time.  Therefore,
   clients, servers, and proxies MUST be able to recover from
   asynchronous close events.  Client software MAY reopen the transport
   connection and retransmit the aborted sequence of requests without
   user interaction so long as the request sequence is idempotent (see
   Section 6.1.2 of [Part2]).  Non-idempotent request methods or
   sequences MUST NOT be automatically retried, although user agents MAY
   offer a human operator the choice of retrying the request(s).
   Confirmation by user-agent software with semantic understanding of
   the application MAY substitute for user confirmation.  The automatic
   retry SHOULD NOT be repeated if the second sequence of requests
   fails.

I figured that persistent connections ought to be reliable, even if you submit a request at the "same time" as the server times out and closes the connection. So, it should transparently do a single automatic retry on a new connection.

But it can't retry POST, because that's not idempotent. (Apparently GET HEAD OPTIONS TRACE PUT DELETE is actually the list of okay methods.)

[itamar]

Thanks for the explanation! I guess I should read that document.

Automatic retries seems like a separate feature, so I think if we want to do that (and the corresponding method restrictions) we can put the logic in a separate class, RetryingAgent or something. I don't think automatic retries should be the default even with persistent connections, given that no doubt for many web applications GET isn't idempotent.

Regardless this ticket is already too big to have retries added to it, so the method restrictions do seem out of scope for now.

[jknight]

It doesn't matter than many web apps have non-idempotent GETs. Browsers rerun GETs without asking the user all the time, so the apps either take care of it internally (by noticing you already did the thing), or else are crazily broken with all current user-agents.

You could support multiple connections reused immediately for already-queued requests, because it's unlikely for the server to decide to close immediately after returning a response that didn't have a 'Connection: close' header.

But supporting cached idle connections without the RFC-suggested single auto-retry-on-fresh-connection seems like a really bad idea, and is just asking for random failure. That logic needs to be in the default persistent-connection-handling code, not an optional bolt-on.

[itamar]

The single auto-retry-on-fresh-connection functionality James is talking about will be in a separate ticket, just because this one is big enough already.

Anyway, ready for review of public API again (and code too, if that looks OK). New APIs include a public HTTPConnectionPool, which has been made more generic. As a result, proxies now support persistent connections, and I explained how HTTPS CONNECT proxies might work. Supporting transparent proxies with persistent connections should also be fairly obvious.

I also fixed a number of bugs involving not noticing disconnects, and not canceling timeouts when connections were removed in certain situations.

[glyph]

New review, new  build results.

[therve]

• There is a small doc conflict.
• Please use assertEqual instead of assertEquals.

• +        self.assertTrue(not self.pool._connections.get(
  +                ("http", "example.com", 80)))
  

  This assert is particularly confusing. Maybe:

  +        self.assertIdentical(None, self.pool._connections.get(
  +                ("http", "example.com", 80)))
  

• +        e, = self.flushLoggedErrors()
  +        self.assertIsInstance(e.value, ZeroDivisionError)
  

  You can pass the exception to flushLoggedErrors directly.

• +        self.assertEqual(transport.disconnecting, True)
  

  I think you should use assertTrue here.

• +    @since: 12.0
  

  We missed the window :/
• It's not clear to me why Proxy is an Agent and not an _AgentBase. No tests fail if I make that change.

• +        assert self._state in ('WAITING', 'TRANSMITTING')
  

  It'd be better if it a proper exception was raised, and that a test was added.

• +    Features:
  +    - Cached connections will eventually time out.
  +    - Limits on maximum number of persistent connections.
  

  You need to indent the list by 1 or it's failed to be parsed by pydoctor. *

  +    @itype maxPersistentPerHost: C{int}
  

  It must be '@type'.

• +        conns = self._connections.get(key)
  +        while conns:
  +            conn = conns.pop(0)
  

  Can you use full variable names here? "connections" and "connection" are much more readable (used in several places).

I like the new API, there is decent documentation and good testing, so I'm happy. Please merge once the small details are fixed.

[itamarst]

(In [33540]) Branching to 'agent-persistent-connections-3420-3'

[itamar]

1. Since flushLoggedErrors doesn't complain if the exception is missing, I think I'll leave it as is.
2. I think the assert is fine, in this situation: it's not doing any checking on public APIs, it's just a sanity check in a private internal method.

[itamarst]

(In [33546]) Merge agent-persistent-connections-3: Support for persistent HTTP/1.1 connections.

Author: itamar, yasusii Review: exarkun, jknight, therve, glyph Fixes: #3420

Add support for persistent connections to Agent and ProxyAgent.

[exarkun]

Some thoughts for the future:

Since flushLoggedErrors doesn't complain if the exception is missing, I think I'll leave it as is.

The reason to pass an exception to flushLoggedErrors is that then it won't return some other logged error that you weren't expecting. In this case, if some other error happens, then you'll get a type error raised by your tuple unpacking. This might be hard for someone who encounters it to understand: it will cause the test to fail in a sort of irrelevant way because the TypeError is a bug in the test, not a bug in the implementation (the bug in the implementation is whatever caused the extra logged exception, which you won't even see in trial output until you look in test.log).

I think the assert is fine, in this situation: it's not doing any checking on public APIs, it's just a sanity check in a private internal method.

I'm pretty sure assert is a violation of the coding standard, and if it's not, that's probably a shortcoming of the documented coding standard. The behavior of assert depends on how Python is invoked. The failure reporting of assert is terrible (it'd be really nice to know what the state is if this ever fails). And application code can't reasonably ever handle this case, since that means writing an exception handler for AssertionError which is a really awful thing to do.

assert doesn't really make any sense in a world where you don't want your entire application to immediately exit.

[itamar]

I actually did end up addressing those two review comments in my final version. The first comment makes a lot sense, but as for asserts, I think we've just swapped it for a RuntimeError - the new code uses the state machine framework, so arguably it's better, so still hard for user code to handle cause if it happens it's a bug in the framework.