Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8320 enhancement closed fixed (fixed)

Remove pipelining from HTTPChannel

Reported by: Cory Benfield Owned by: Amber Brown (HawkOwl) <hawkowl@…>
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch: nopipeline-8320-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

Related to the HTTP/2 work.

Several of the difficulties implementing the HTTP/2 support in Twisted have boiled down to the fact that the t.w.h.Request object supports HTTP/1 pipelining in its own source code. It does this by having a .transport property to which it directs all its writes: when a response is in the pipeline queue, that .transport property is actually a StringTransport, which gets swapped out for the actual transport later.

HTTP/2 doesn't do pipelining: no response can be "queued". That means that we always want a Request object to write to the channel. However, a queued Request cannot write to its channel.

There are lots of ways to fix this problem, but the most fundamental is to simply disable concurrent processing of pipelined requests. This means that the .transport property on Request can no longer *ever* be a StringTransport, and, more importantly, that the Request is now always safe to write directly to the channel.

This has several other side benefits:

  1. RFC 7230 says that servers that support pipelining must only process requests using safe HTTP methods concurrently. Twisted's implementation of HTTP/1 pipelining violates this clause: it will always process pipelined requests concurrently, regardless of method. That means that, in practice, almost any twisted.web-based web application *will* violate RFC 7230 if a client pipelines requests to them. That's bad. In addition, because this is spec-violating behaviour, we do not need to follow the normal deprecation cycle.
  2. Incautious consumers of twisted.web will not be aware that pipelining is a possibility, and will therefore put themselves at risk of resource exhaustion. For example, consider Daphne, the Django Channels web server. Daphne dispatches request information across a non-ordered transport. If Daphne receives one request that takes an extremely long time to process, followed by several requests that take little time to process but generate a lot of data, it is possible to force Daphne to buffer substantial amounts of data while it waits for the response to the first request. This is almost certainly only possible because Daphne isn't designed with awareness of pipelining.
  3. Twisted doesn't really talk anywhere about its support for pipelining, meaning that it's hard to discover that it's there if it's causing problems.
  4. twisted.web provides no mechanism for *disabling* pipelining.

For all of those reasons, this patch will remove pipelining support from twisted.web. twisted.web will still support a client pipelining its requests to twisted.web, but it will no longer process them concurrently. This avoids the risk of breakage, and fits the mental model users have of twisted.web much better. This change will somewhat reduce throughput of a twisted web server if it was making active use of pipelining, but that throughput came at the cost of correctness and so was always something of a false economy. Additionally, pipelining is extremely rarely used in the HTTP/1.1 world because of the many breakages it causes, so in practice twisted.web rarely encounters clients that pipeline.

This patch needs extensive review and feedback. In particular, I have removed an argument from the t.w.h.Request constructor. That change itself may be backward incompatible in a way that the Twisted devs are unhappy with, so I'm happy to return that keyword argument but to warn if it's being used.

Otherwise, we should also try to reach out to those consumers of twisted.web we know about to ensure that they're aware of this change.

Attachments (2)

8320_1.patch (50.7 KB) - added by Cory Benfield 3 years ago.
First draft patch
8320_2.patch (22.5 KB) - added by Cory Benfield 3 years ago.
Second draft patch

Download all attachments as: .zip

Change History (29)

Changed 3 years ago by Cory Benfield

Attachment: 8320_1.patch added

First draft patch

comment:1 Changed 3 years ago by Glyph

Author: glyph
Branch: branches/nopipeline-8320

(In [47363]) Branching to nopipeline-8320.

comment:2 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Cory Benfield

Thank you for your contribution, Lukasa. Nominally this is "removing" something, i.e. pipelining, but in fact I think it makes the API for twisted.web more consistent.

I do have some feedback :).

  1. The tests are failing like crazy on python 3. Because you forgot to remove a debug print. Oops. Please remove it. Twistedchecker is similarly unhappy with that.
  2. The modification of the signature in Request.__init__ and Site.requestFactory are obvious compatibility policy violations. Please deprecate the queued parameter, don't just delete it. It might even be sensible to leave in the StringTransport behavior, in case anyone was using that in a test or something; just un-hook it from the behavior in HTTPChannel (i.e. have HTTPChannel explicitly refuse to .process() a queued request, and instead wait until the request is ready). In requestFactory's case, you can have the caller ask to have a different signature used by using @provider(INewAwesomeRequestFactory) on a function, or @implementer(INewAwesomeRequestFactory) on a class.
    1. I'd appreciate it if you could split out the change to all the callers of Request to not pass a hard-coded False for queued (along with, at the same time, issuing the actual DeprecationWarning for queued) into a separate follow-up ticket. It creates a lot of chaff.
  3. You are adding, but not documenting, _handlingRequest and _dataBuffer attributes; please add @ivar docstring fields to HTTPChannel.
  4. except AttributeError: pass is an antipattern. If, for example, self.transport is an AttributeError, and not transport.pauseProducing, you want to see that traceback. If resumeProducing touches some inconsistent internal state and gets an AttributeError, you want to see that too. This pattern, in the two places where you've used it, should be replaced by an idiom like getattr(self.transport, "pauseProducing", lambda: None)(). Better yet, do self._producer = IPushProducer(self.transport, _NoPushProducer()) where _NoPushProducer is a null implementation of IPushProducer.

Please resumbit when you've dealt with all of this.

Since adiroiban voiced some concerns about the compatibility impliciations of simply removing pipelining with no deprecation period on IRC, let me bundle addressing that into my review. It is my view that (as long as you fix the egregious signature breakages), this is a totally fine change with respect to the policy.

  1. Given your well-motivated changes explained in your first point, you're on pretty firm "gross violation of specifications" ground when it comes to the behavior change.
  2. By far the biggest motivation of the spirit of the policy is to allow people to be able to upgrade without concnern for breakages. The nature of pipelining is that de-supporting it can only break one thing: if your application has clients which send it one request which acquires a semaphore and then require a pipelined request to release it before the first response can proceed. Given that such a request would not be a "safe request" under rfc7231's definition, regardless of method, an application would have to be really aggressively violating the HTTP spec to get into this condition in the first place, not to mention that in the presence of compliant proxies or caches, it would already be broken. We are generally pretty lenient about potential behavioral breakages which don't actually break anything, but just reveal a pre-existing brokenness in more contexts. (Of course whether this is a good idea or not depends on how many consumers are doing it, but whether it's a policy violation or not is down to whether the prior behavior was actually correct in practice under different constraints.)
  3. Furthermore, pipelining itself is a feature which is very nearly "incompatible" in the Twisted policy sense, with itself. As recently discussed elsewhere, if you want to inspect the TLS properties of a request, the only way to do this presently is request.transport.getPeerCertificate(); for clients there's getClientIP, but if you want to know your own host address in a Resource, there's no way to do it other than request.transport.getHost(). This will work most of the time! Unless, of course, your client is sending requests quickly enough to trigger pipelining, in which case all these attributes will disappear from your transport and you have no way of retriving them at all. So by removing this code, you actually make Request more self-consistent and have a more reliable interface.
  4. Building on point 2, most clients that would be triggering it would be doing it by simply blindly dumping requests onto the socket quickly, which means that whether pipelining was triggered or not would be highly non-deterministic, because if Twisted had started sending the response to the first request because it could compute it before the second arrived, this code path wouldn't have been hit. So for all but the most egregioulsy poorly-architected server applications, it's literally indistinguishable from a network condition you have to handle anyway.

Given all those points I think we are well within the confines of the compatibility policy and may happily proceed.

comment:3 Changed 3 years ago by Cory Benfield

It might even be sensible to leave in the StringTransport behavior, in case anyone was using that in a test or something; just un-hook it from the behavior in HTTPChannel (i.e. have HTTPChannel explicitly refuse to .process() a queued request, and instead wait until the request is ready).

glyph and I briefly chatted about this in IRC. I pointed out that while that approach works fine for *this patch*, it doesn't work fine in the long run because I want to follow this patch up with a change that has Request unconditionally write to the channel rather than the transport. That doesn't work if people's tests are *expecting* the queueing behaviour.

glyph agreed that we should disregard that suggestion, so I will.

Changed 3 years ago by Cory Benfield

Attachment: 8320_2.patch added

Second draft patch

comment:4 Changed 3 years ago by Cory Benfield

Keywords: review added
Owner: Cory Benfield deleted

Ok, I've uploaded a new patch that I believe follows glyph's requirements.

comment:5 Changed 3 years ago by hawkowl

Branch: branches/nopipeline-8320nopipeline-8320-2

comment:6 Changed 3 years ago by Amber Brown (HawkOwl) <hawkowl@…>

In 2d39ccad:

apply patch from lukasa, refs #8320

comment:7 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to Cory Benfield

Hi Cory, thanks for the patch.

  1. https://codecov.io/gh/twisted/twisted/commit/2d39ccad9d8a9b86229dd7fb1ce47ca841b730be#747769737465642F7765622F687474702E7079-1812 The queuing doesn't seem to be tested, which makes me uneasy.
  1. There's a lot of removed tests, but it doesn't seem that what's there makes sure that our quasi-pipelining works.
  1. Please don't outright remove def noLongerQueued(self):, deprecate it and make it a no-op.
  1. A newsfile is required. I don't know how detailed it needs to be though, but explaining what it changes for deployments sounds like a good idea.
  1. There doesn't seem to be coverage for a theoretical INonQueuedRequestFactory... shouldn't the current RequestFactory be that? https://codecov.io/gh/twisted/twisted/commit/2d39ccad9d8a9b86229dd7fb1ce47ca841b730be#747769737465642F7765622F687474702E7079-1679

Otherwise, the builds are green and the code looks alright, please submit for rereview when you've fixed the above. Thanks again!

comment:8 Changed 3 years ago by Cory Benfield

Thanks hawkie, I'll revise to address the coverage concerns. One note though:

There doesn't seem to be coverage for a theoretical INonQueuedRequestFactory... shouldn't the current RequestFactory be that?

No. Due to Twisted's deprecation policies, I can't actually remove the kwarg for queueing: there's a sentinel there to warn about it. That means that the new interface isn't actually *implemented* by anything. That's a good spot though, we should add a test to validate that it does, you know, actually work.

comment:9 Changed 3 years ago by Cory Benfield

Keywords: review added
Owner: Cory Benfield deleted

Ok, I've updated my fork with the changes requested by hawkowl, all pushed to the branch of the same name as currently here. It should be a simple matter to pull those in. Back for review.

comment:10 Changed 3 years ago by Cory Benfield <lukasaoz@…>

In bb4b0e8:

Add newsfile for #8320.

comment:11 Changed 3 years ago by hawkowl

Pulled in Lukasa's changes, spun the builders...

comment:12 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to Cory Benfield

Hi Cory, thanks for your continued work on this.

  1. The Python3 tests are failing; you'll need to use the decorators instead of the class advice style.
  1. NoLongerQueued's docstring doesn't mention that it doesn't do anything; can you change it to say it's there purely for backwards compat?
  1. The fallback buffering in raw mode (that is if pauseProducer is not implemented) doesn't seem to be tested, but the other one is. Is this a reachable codepath? https://codecov.io/gh/twisted/twisted/src/bb4b0e883e01642070715130a88fbfca927e098f/twisted/web/http.py#L1838
  1. Could you point me to, or add a comment to, the tests which exercise the pipelining as implemented here?

Please fix the above issues and submit for rereview. Thanks again.

comment:13 Changed 3 years ago by Cory Benfield

Keywords: review added
Owner: Cory Benfield deleted

Ok, I've pushed some new commits that should address the problems here. Back for review!

comment:14 Changed 3 years ago by hawkowl

I pulled the changes into the Twisted branch; waiting for GitHub to catch up...

comment:15 Changed 3 years ago by hawkowl

Spun the builders.

comment:16 Changed 3 years ago by hawkowl

Branch: nopipeline-8320-2nopipeline-8320-3

comment:17 Changed 3 years ago by Amber Brown (HawkOwl) <hawkowl@…>

In 21ca486:

apply patch from lukasa, refs #8320

comment:18 Changed 3 years ago by Amber Brown (HawkOwl) <hawkowl@…>

In 63fb5981:

Add newsfile for #8320.

comment:19 Changed 3 years ago by hawkowl

Branch: nopipeline-8320-3nopipeline-8320-2

comment:20 Changed 3 years ago by Amber Brown (HawkOwl) <hawkowl@…>

Owner: set to Amber Brown (HawkOwl) <hawkowl@…>
Resolution: fixed
Status: newclosed

In 1d69e04:

Merge nopipeline-8320-2: Remove parallel HTTP/1.1 pipelining

Author: lukasa
Reviewers: glyph, hawkowl
Fixes: #8320

comment:21 Changed 3 years ago by warner

Incidentally, this seems to have broken the magic-wormhole tests (https://github.com/warner/magic-wormhole/issues/62). It doesn't appear to affect normal operation, but at least one of the tests is hanging because the websocket messages aren't getting through. I don't think we're doing anything special in the test suite, so I'm still investigating to see what's happening.

comment:22 in reply to:  21 Changed 3 years ago by Glyph

Replying to warner:

Incidentally, this seems to have broken the magic-wormhole tests (https://github.com/warner/magic-wormhole/issues/62). It doesn't appear to affect normal operation, but at least one of the tests is hanging because the websocket messages aren't getting through. I don't think we're doing anything special in the test suite, so I'm still investigating to see what's happening.

Dang. I'm really sorry to hear that. Please keep us posted with an explanation of what exactly broke; I imagine if you were relying on pipelining there should also be a fix on your end, but perhaps there's something we can address in Twisted as well to avoid interfering with other applications.

comment:23 Changed 3 years ago by Shane Hathaway

I appreciate all the work going into HTTP/2 (YAY!), but sadly, this change broke txsockjs. This line of allContentReceived() is what broke it:

self._producer.pauseProducing()

txsockjs relies on a copy of twisted/web/websockets.py:

https://github.com/DesertBus/sockjs-twisted/blob/master/txsockjs/websockets.py

It looks like websockets.py is not actually part of Twisted yet; I wonder where exactly it came from. In any case, the code that broke is at the bottom of the file. It changes transport.protocol to a _WebSocketsProtocol and returns NOT_DONE_YET, expecting the new protocol object to take control of the stream. A comment in the file acknowledges it is relying on undocumented functionality and references issue #3204. Almost everything still works as before; the only problem is that the producer is simply paused.

I can see a couple of reasonable choices:

1) We could say that NOT_DONE_YET was never documented to allow the Resource to both write and read the channel; it was only documented to allow the Resource to write the channel asynchronously. Therefore this change only broke applications that relied on undocumented functionality. Code that is now broken can work around the problem by calling IPushProducer(transport).resumeProducing() or some variation of that.

2) In server.py, where it checks "if body == NOT_DONE_YET", we could resume the channel, thereby restoring the undocumented behavior. I'm not sure how I feel about that solution.

comment:24 Changed 3 years ago by hawkowl

The first option is what we've settled on in things like Autobahn. self.transport.resumeProducing() should work on all Twisteds, but you could look before you leap, I guess, to make sure.

comment:25 in reply to:  24 ; Changed 3 years ago by Shane Hathaway

Replying to hawkowl:

The first option is what we've settled on in things like Autobahn. self.transport.resumeProducing() should work on all Twisteds, but you could look before you leap, I guess, to make sure.

Yup. I've submitted a pull request to txsockjs.

comment:26 Changed 3 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

comment:27 in reply to:  25 Changed 3 years ago by mark williams

Replying to Shane Hathaway:

Replying to hawkowl:

The first option is what we've settled on in things like Autobahn. self.transport.resumeProducing() should work on all Twisteds, but you could look before you leap, I guess, to make sure.

Yup. I've submitted a pull request to txsockjs.

You might be interested in https://github.com/markrwilliams/txdarn which works with both the latest Autobahn and Twisted.

Note: See TracTickets for help on using tickets.