Reverse protocol handling for WebSocketResource

[warner]

I have a set of unit tests (in https://github.com/warner/magic-wormhole) that recently started failing when I combine autobahn-0.13.1 and Twisted trunk. It looks like https://github.com/twisted/twisted/commit/4f9e36d8ad21f9d73369f4d506bab30f61c2ad71 (landed 29-Mar-2016) is where the problem started (that's the first revision after Twisted-16.1.1 was branched off that causes my project's tests to fail).

The commit says:

Merge hide-request-transport-8191-5: Hide the transport backing the HTTPChannel object from twisted.web Resource objects.

And my tests are using WebSocketResource to put a websocket on a specific URL. I haven't tracked down the specific failure yet, but it seems like the server is having an internal error when the client attempts to connect to it, or the server isn't accepting the connection at all.

The relevant Twisted bug is at https://twistedmatrix.com/trac/ticket/8191 .

When I get some time (maybe next week), I'll figure out how to run autobahn's test suite against various versions of Twisted and see what happens.

[meejah]

Hi @warner, the test-suite can be run with tox so you could tweak one of the existing environments for the "right" Twisted version, probably.

[oberstet]

this: https://github.com/crossbario/autobahn-python/blob/master/autobahn/twisted/resource.py#L127

[warner]

Ah, thanks. tox -e py27-twtrunk looks like it ought to do the right thing, but all the tests pass on my box, so I think magic-wormhole must be doing something unusual (maybe correct and uncovered by autobahn's tests, or maybe wrong).

[oberstet]

Connect the transport to our protocol. Once #3204 is fixed, there may be a cleaner way of doing this.

http://twistedmatrix.com/trac/ticket/3204

8 years old bug.

The background is: to add a Twisted Web resource that runs WebSocket, we (Autobahn) have to get at the full initial HTTP request bytes - which have been eaten away by Twisted Web before.

I consider the code in Autobahn a "gentle hack" .. due to lack of appropriate interfaces in Twisted (and me, and that time, not having the patience to go through months of discussions to land something on Twisted;)

[oberstet]

Twisted has made an attribute private that was formerly public?

twisted.web.http.Request.transport was deprecated in Twisted 16.2.0. Call directly into the Request object instead.

I'm not sure what this is supposed to mean;)

Anyway: Twisted should IMO have an API so that I can take over the full HTTP request on a Web resource .. all the bytes from byte 0 on ..

[oberstet]

Btw: this is a bad one .. it'll basically break almost all Crossbar.io deployments ..

[oberstet]

@warner could you please post the traceback you get from your tests here?

[warner]

File "/Users/warner/stuff/python/twisted/twisted/internet/defer.py", line 588, in _runCallbacks
            current.result = callback(current.result, *args, **kw)
          File "/Users/warner/stuff/tahoe/magic-wormhole/ve/lib/python2.7/site-packages/autobahn/twisted/websocket.py", line 203, in forwardError
            return self.failHandshake("Internal server error: {}".format(failure.value), ConnectionDeny.INTERNAL_SERVER_ERROR)
          File "/Users/warner/stuff/tahoe/magic-wormhole/ve/lib/python2.7/site-packages/autobahn/websocket/protocol.py", line 2767, in failHandshake
            self.sendHttpErrorResponse(code, reason, responseHeaders)
          File "/Users/warner/stuff/tahoe/magic-wormhole/ve/lib/python2.7/site-packages/autobahn/websocket/protocol.py", line 2779, in sendHttpErrorResponse
            self.sendData(response.encode('utf8'))
          File "/Users/warner/stuff/tahoe/magic-wormhole/ve/lib/python2.7/site-packages/autobahn/websocket/protocol.py", line 1192, in sendData
            self.transport.write(data)
          File "/Users/warner/stuff/python/twisted/twisted/web/http.py", line 2055, in write
            assert self._sendState == _ChannelSendState.SENT_HEADERS
        exceptions.AssertionError:

It feels like the websocket code is attempting to do a channel.write(), and the Twisted patch changes the rules to say that you're only allowed to do .write() after doing .writeHeaders() (there's now a state machine, that strictly alternates between writing response headers and writing response/body bytes).

I don't think that's quite it, though, because I'd expect the websocket code to be doing channel.transport.write(), not channel.write(). I'd expect to see deprecation warnings, but not outright errors.

There might be something else in my code that's triggering an error at an unusual time, causing the websocket protocol to shut down at an unusual time (maybe before connection negotiation has completed?), and then maybe some infrequently-used error path is writing to the wrong place?

[Lukasa]

Leaping in quickly: that makes sense.

I wouldn't be surprised if the websocket code grabs request.transport, which is now the same as request.channel. That's why we're getting into trouble with this state machine. If autobahn changes that code to request.channel.transport that'll continue to work, I think.

[warner]

Oh, wow, I missed that: they not only deprecated access, they completely redefined what .transport is (on line 620 of the new file):

-            self.transport = self.channel.transport
+            self._transport = self._channel

I wonder if that was intentional, or if transports and channels were so similar (or were just always used in the same way) that it made sense to make that change.

[Lukasa]

Heh, that wasn't a "they", that was me. =)

The concern here was that quite a lot of twisted.web code would grab the backing transport directly, which breaks with HTTP/2 (because you can't just write bytes onto the transport and expect that to work). The goal, then, was to pass all writes through the channel, which "owns" the transport.

Clearly, Autobahn was doing what the rest of twisted.web did (which was just grab the transport out of the request, without considering what the channel might want to do). Again, Autobahn can just grab the transport out of the channel: that should work on all Twisted versions.

[warner]

Hah :).

I realized I probably truncated the traceback here.. it actually starts in autobahn.websocket.protocol processHandshake(), then into autobahn.twisted.websocket.py _onConnect(), then wanders through defer.py before hitting forwardError.

I attached the full traceback to the twisted bug (8191).

[warner]

Hm, maybe there's something AutoBahn could do to put the channel into the SENT_HEADERS "let me write bytes now" state and then just leave it there forever.

[Lukasa]

@warner AutoBahn could, but I don't really know why it'd bother when it can just grab the actual transport instead. request.channel.transport will work on all Twisteds.

[warner]

Ah, cool.

[oberstet]

Weell. I am a bit concerned about all this stuff going on before Autobahn can grab the transport. Because of performance: WebSocket connection establishments / sec.

Note: I don't care at all about HTTP/2 ..

I am thinking about if we can do it reverse: Autobahn has the listening socket, and only if it doesn't match a configured WebSocket path, it'll hand over to Twisted Web ..

[Lukasa]

@oberstet So that's an understandable concern, but it's already true. We didn't add any extra work here. =)

That said, you'd almost certainly get a huge performance boost by grabbing the transport earlier. =) If you're interested in a way you could do it, you might want to check out the _GenericHTTPChannelFactory and _GenericHTTPChannel objects in twisted.web.http trunk: those are precursors to being used for basically that purpose HTTP/2, and might be suitable for extension or copying in Autobahn.

[oberstet]

Another reason we probably will go "reverse": the main use case for this feature in Autobahn (having WebSocket on a Twisted Web resource) is in Crossbar.io. And we want to have the ability to have both WebSocket and RawSocket (a WAMP transport) on the same listening port. And the latter is distinguished from WebSocket by a first magic octet (x7f) that isn't valid for any HTTP. This is by design (of RawSocket) ..

[oberstet]

I'd expect that _GenericHTTPChannel will bail out hard on a first octet x7f?

[Lukasa]

@oberstet All of this seems like a really good reason to prioritise having a "pluggable" protocol in Twisted, letting you swap them in and out in a defined way (as you suggested initially).

I'd expect that _GenericHTTPChannel will bail out hard on a first octet x7f?

Right now it would because it defaults to HTTP, but you could swap out the approach. It basically just overloads dataReceived and swaps some backing stuff around, so if you wanted to you could do something very similar and allow first octet \x7f (or indeed anything you want).

[oberstet]

A "pluggable" protocol sounds desirable .. I agree. At least for Crossbar.io, ideally it would work like this:

• CB listens on port 80
• if the first octet is x7f, make it a RawSocket protocol
• else, parse the first HTTP request line to get the path
• if the path is configured for WebSocket, hand over to WebSocket protocol
• else, hand over to Twisted Web

[oberstet]

FWIW, I now realize that the HTTP/2 thing in Twisted Web now, even if we change how to get at the transport, will shuffle each and every write through its code and state machine.

This is an absolute no-go for Atuobahn because of performance.

So, the real solution to this issue for AB is to bypass Twisted Web in the first place altogether (the "reverse" thing above) ..

[oberstet]

@Lukasa when is this goin to land on twisted? I mean, released ..

[Lukasa]

@oberstet No it won't.

If you grab the actual transport (request.channel.transport at the moment, somewhat more complex with HTTP/2), you'll be able to write directly to the wire. Not that it matters anyway, because right now websockets-over-HTTP/2 isn't specified, so you just shouldn't need to care.

[oberstet]

No, because of the reasons above, we'll do it "reverse" .. that fixes this, and allows us to co-listen for WebSocket and RawSocket.

As said, I have no intention / use case for HTTP/2 - so this is basically a drag .. but the good thing: we'll get the co-listening thing in CB;)

[oberstet]

@Lukasa when does it land?

[Lukasa]

It lands when it lands, sadly. We don't have a deadline, the work is ongoing. The next patch in the sequence is 8194 which has some outstanding review work that still needs doing, and then we'll need to merge 8188. The goal for those is Soon (tm), but it'll take as long as it takes.

[oberstet]

Mmh, ok, then we'll probably be better for now to pin Twisted at <= 16.1.1 for Autobahn and Crossbar.io ..

[oberstet]

For the record (and others coming along / wondering): I do know WebSocket quite well, and I do know about the (failed) WebSocket-over-HTTP/2 efforts. In fact, I participated there. The HTTP/2 IETF WG has been .. complicated/surprising from the WebSocket IETF WG point of view (in my experience).

[warner]

Seems like a good emergency patch. It'll make things tricky for my project (which I just switched to using Autobahn a few days ago), in a few months when the next Twisted versions come along, that contain important bug fixes or whatever.

I'm using a WebSocketResource so I can serve two different APIs to the same rendezvous service from the same port (one based on HTTP POSTs and Server-Sent-Events, the other based on WebSockets). I think I can use the reverse form without problems. Would the new setup look something like this?:

root = resource.File() # holds the non-WebSocket resource tree
f = WebSocketResourceFactory(MyWebSocketProtocol, http_root=root)
endpoint.listen(f)

And then any ws:// URLs that land on that endpoint would cause MyWebSocketProtocol to be spun up, and any HTTP/HTTPS URLs would get passed to root.getChild?

I don't need multiple WebSocket endpoints in the same site, so that'd work for me, although obviously getting the version constraints right will be important (hopefully there'll be a new version of Autobahn, with this new API, that I can depend upon before the next Twisted release comes out).

Do you have users who need to dispatch to different WS protocols depending upon the URL? Or, can that be done from within the Factory? If not, maybe add this: parse the request, call factory.buildProtocolFromRequest(addr, request), that defaults to just calling self.buildProtocol(addr), but apps can override the first one to build different protocols depending upon request.path.

[oberstet]

@warner Yes, approximately. If it wouldn't be for:

And then any ws:// URLs that land on that endpoint would cause MyWebSocketProtocol to be spun up, and any HTTP/HTTPS URLs would get passed to root.getChild?

We need to be able to run multiple WebSocket endpoints (under different HTTP paths).

Do you have users who need to dispatch to different WS protocols depending upon the URL?

Yes. Eg https://github.com/crossbario/autobahn-python/blob/master/examples/twisted/websocket/multiproto/server2.py

I don't have a nice answer yet. We'll likely have to do a shallow parsing of the first HTTP request line, and do the URL mapping ourselfs (is it a WebSocket path? if not, handover to Twisted Web resource tree. If yes, we'll handle it ourself).

But if Twisted Web is forcing each and every transport write through a HTTP/2 state machine, this sucks big time from my perspective (no gain, but drain). On the other hand, its "good" that it pops up now (as in Crossbar.io, we not only want to run WebSocket, but also RawSocket on the same port - a feature that is missing right now). IOW: this forces us to dig deeper now;)

But anyways:

• we will find a way to still allow running WebSocket and Twisted Web resource trees on one port
• we'll make sure to address this soonish (get rid of pinning Twisted as soon as possible - which is bad obviously)

[warner]

Great! Thanks for diving into this so quickly.. I'm really digging Autobahn so far, it was a big improvement over my Server-Sent-Events approach.

[oberstet]

Thanks for the nice words=) Cool to have you as a user .. I mean, Buildbot, Tahoe .. gems! And BB9 is gonna use Autobahn/Crossbar.io at the WAMP level .. pretty exciting;)

[oberstet]

@warner this is how the "reversed" thing looks like https://github.com/oberstet/autobahn-python/blob/multisocket/examples/twisted/multisocket/echo/server.py#L75

it'll work with all Twisted versions, shield from the HTTP2 fallout, does support Web+WebSocket+RawSocket all on 1 listening port and is faster.

no more WebSocketResource at all.

does that work for you?

[glyph]

@oberstet - it seems like this would break idioms like authenticating access to websocket resources, or using a different websocket factory for different users, or based on a cookie; things like that. It also breaks the ability to compose multiple websocket applications in a single process via delegation, as opposed to hard-coding the integration up front in a configuration file.

There appears to be a lot of misguided premature optimization going on in this thread; for example, how "will shuffle each and every write through its code and state machine" and this is a performance problem. You can't possibly have performance tested this, because your assumption is incorrect, so how do you know it's a performance problem? :) PyPy tends not to penalize low-overhead abstractions like a simple state machine.

It would also seem to break the ability to speak HTTP/2 and websockets on the same port, because it would interpose between the necessary NPN handshaking interface and the Site object. Websocket clients themselves will of course be http/1 only, but HTTP/2 is a huge optimization in terms of latency for browsers that support it; and latency usually dominates in browser/server conversations. So if we're concerned about performance, this is the place to do it.

All in all it seems like the right solution is to address http://twistedmatrix.com/trac/ticket/3204 properly, or to continually be hacking around Twisted's object model and breaking edge-cases. I realize that it's a bit of work to contribute to Twisted but avoiding it in the first place has not had a great result in this case :).

[oberstet]

@glyph You left out the elephant;)

We have a need to run RawSocket, WebSocket and Twisted Web all on one port. Only the latter 2 are HTTP like at all. The first one starts with a 7F octet which is invalid HTTP by design.

So I am looking for a clean way of serving my own protocol, and only if I wish so, switch over to Twisted Web. Is that possible?

Btw: yes, I measured overhead (WebSocket opening handshakes / sec) like 2 years ago .. and it definitely is slower when running under Twisted Web. And it will only get worse with this state machine stuff for HTTP2. And disagree about the usefulness of HTTP2 - for the stuff we do. SPAs are cached once, and then there isn't much of HTTP anymore. If one needs HTTP2 for serving lots of smallish static resources, nothing beats Nginx anyway. More: of course you can switch WebSocket based protocols based on WebSocket opening handshake info: https://github.com/crossbario/autobahn-python/tree/master/examples/twisted/websocket/multiproto

Contributing to Twisted: I was hoping that (partially) paying @hawkowl to work on Twisted stuff would count;) Now, she probably has more the Twisted perspective and needs on radar rather than Autobahn/Crossbar.io =( Anyway, I have way too much to do ..

[meejah]

A bit of brainstorming here, hacking on top of @oberstet's example above; is this more-like something that could go in Twisted proper? Essentially it just re-organizes @oberstet's code a bit, around an (order-matters) list of (predicate, protocol_factory) pairs.

@glyph I read through 3204, and presume that my _switch_protocols method could be something like setProtocol wants...? I think it does need the "data to pass on" argument to be flexible enough? The "predicates" idea probably needs more thought too ... :/

https://github.com/crossbario/autobahn-python/commit/2aa1d0d89669be005ba2344dcd88e01c22109a2c

[meejah]

Hmm, okay more reading of 3204 seems you're more looking for the actual protocol-switch to be more like self.transport.switchProtocol(new_protocol, leftover_bytes)...? (i.e. rather than wrapping it)

[glyph]

Contributing to Twisted: I was hoping that (partially) paying @hawkowl to work on Twisted stuff would count;)

I'm not keeping score here :). Of course your contributions to @hawkowl's time on Twisted are deeply appreciated. It's just that, technically, on this specific issue, a fix in Twisted would be the right way forward.

Now, she probably has more the Twisted perspective and needs on radar rather than Autobahn/Crossbar.io =(

Actually, she did specifically raise this issue as a potential problem for Autobahn as we were discussing it; we tried to come up with a solution that was as low-impact to Autobahn specifically as possible. I think the sticking point here is just that this code path happened to be un-covered by tests?

[glyph]

Hmm, okay more reading of 3204 seems you're more looking for the actual protocol-switch to be more like self.transport.switchProtocol(new_protocol, leftover_bytes)...? (i.e. rather than wrapping it)

Mutation is always more efficient than delegation. PyPy saves you a bit on some kinds of delegation, making them almost as cheap, but at the very least you have the memory for an extra pointer floating around. Share all the mutable state! :-(

[glyph]

@meejah @oberstet - btw, Tubes took this problem into account early in its design. See http://twisted.github.io/tubes/docs/tubes.tube.Diverter.html for example.

[glyph]

To answer the main concern here, for @oberstet:

So I am looking for a clean way of serving my own protocol, and only if I wish so, switch over to Twisted Web. Is that possible?

That is definitely possible. But it also, by definition, leaves out the possibility of properly treating your protocol as an HTTP resource. Instead it's a hack to trade off the resource abstraction against wire speed.

However, I should note that this kind of abstraction-breaking hackery is basically a best practice in the HTTP world. Putting nginx in front of things and manually maintaining a URL mapping in its configuration file which is specific to and tightly coupled with your application, yet managing it separately, is totally the normal way most web services are deployed. Everybody denormalizes and shards all of their data in their SQL databases, and manually (and usually incorrectly) manages caching into NoSQL or memory caches beyond that.

In the face of these widespread obviously broken patterns, maintaining a tiny separate URL mapping for a few websockets on an arguably dedicated service is an infinitesimally small offense :). I wouldn't want something like this in Twisted itself, because it breaks its abstraction model, but allowing you to break the abstraction when necessary is part of the layered nature of Twisted's API, so there's no hard reason why Autobahn shouldn't do this.

[oberstet]

@glyph The Diverter seems exactly what I am looking for! Just for Twisted (Web) =)

Here is what I came up now (which works ... but it is a new hack;)

https://github.com/oberstet/autobahn-python/blob/multisocket/examples/twisted/multisocket/echo/README.md

I think something in the direction what @meejah suggests is even better! Less "ad hoc", and a user can modify the mapping from Request-URI to protocol more flexibly and also after construction.

Mmh. Maybe we need both? I mean, having something like Autobahn's WebSocketResource, but using officical Twisted machinery and where Twisted Web is owning the listening socket and something like the Diverter or MultiSocket, where the latter is sitting on the socket, and Twisted Web gets switched over to.

[oberstet]

@glyph This is totally OT rgd this thread, but going to Zero Inbox, Email Bankruptcy and "We’re In This Together" is awesome=) Absolutely. I give up on catching up;) https://glyph.twistedmatrix.com/2016/04/email-isnt-the-problem.html

[Lukasa]

I feel like we haven't made something clear here: the only time bytes pass through the HTTP/2 state machine is if you're actually using HTTP/2. If you don't negotiate HTTP/2 in the ALPN connection you pay no cost for the HTTP/2 support. Given that WebSockets-over-HTTP/2 does not exist, the only possible place that HTTP/2 could be a performance problem for Autobahn is in the actual serving of resources over HTTP/2, in which case some early benchmarking from me suggests that the various HTTP/2 efficiency gains lead to improved performance over HTTP/1.1 in Twisted.

[meejah]

@oberstet FWIW, I quickly rebased the patch from Twisted's 3204 to Twisted trunk, and further hacked my example above so that it does self.transport.switchProtocol(..) and it works fine.

So, if that gets patched up and landed in twisted then potentially something like the hacked up MultiProtocol/MultiProtocolFactory (er, SwitchableProtocol I called it above) thing could also go into Twisted, and then all Autobahn would need is a factory-function that creates an appropriate instance of a MultiProtocolFactory (i.e. all the stuff I put in server2.py, and the predicates or something like them).

[tomprince]

@oberstet I understand that crossbar.io wants to control the entire interaction, and only delegate to twisted.web for HTTP interaction. But personally, I have no interest in giving autobahn control at that level. And it isn't necessarily even possible to do that ... if I am writing a plugin for some project that just expects me to provide a t.w.r.Resource and hooks it up somewhere, then I'm not in a position to let autobahn take over the top-level protocol.

[oberstet]

@tomprince Yeah, I do understand that. Which is the reason I think that ideally we (Twisted + Autobahn) find a way to support both (having Autobahn take control and handover to Twisted Web and having Twisted Web take control and handover to Autobahn).

@Lukasa That is certainly good (not shuffling bytes through a HTTP2 state-machine), but Crossbar.io needs are: we have to take control, since we want RawSocket on the same port. Plus: we don't care about HTTP2 at all. The performance of Twisted Web on HTTP/1.1 serving static assets is limited by Twisted Web not employing in-memory caching of static assets from files. https://github.com/crossbario/crossbarexamples/tree/master/benchmark/web. Then, HTTP2 handshake is more complex than HTTP1, so WebSocket opening handshakes / sec will likely be slower .. but I haven't measured that. What I did measure is: WebSocketResource on Twisted Web HTTP1 is slower on WS handshakes/sec than pure WS (Autobahn).

[Lukasa]

So I'm totally understanding of crossbar's needs: I'm not telling you what you should and shouldn't have. =) I just want to make sure you're aware of what the facts are with Twisted's HTTP/2 support, so you can make the best decision possible. =)

When you say the HTTP/2 handshake is more complex than HTTP/1, what do you mean exactly?

[oberstet]

@Lukasa The (sad) truth is: neither WebSockets-over-HTTP/2 nor WebSocket-MUX exists. This sucks, but the relevant IETF WGs failed to reach consensus on both. The reasons for this from my perspective are at least partially politics of big G. They've been on both parties. Anway.

From my understanding, the HTTP2 handshake at least needs to establish one HTTP2 stream to actually shuffle data, after the actual HTTP2 Upgrade (both peers agreed to talk HTTP2) already happened.