WebSocket server support

[therve]

Twisted should support the WebSocket protocol on the server side.

See  http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol-16

[therve]

(In [27790]) Branching to 'websocket-4173'

[exarkun]

Note the newer draft:  http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol-68

[therve]

It's probably not ready yet, but I'd like some feedback.

One notable feature missing is binary frame support. As browsers don't support it for now, I don't think it's critical. The current architecture should make it trivial to implement.

I added an example, and a documentation page. The latter is fairly rough, but I hoped it would help to understand what's implemented. The example worked for me with a recent chromium build.

Please review!

[jesstess]

Thank you for providing examples and documentation right off the bat! Here's some largely documentation-related feedback.

• websocket.py:
  • WebSocketHandler needs a class docstring
  • need an @type on all the params and ivars
  • "a too big frame" ==> "too big a frame"
  • A comment on why MAX_LENGTH is the number it is would be nice. It's a power of 2 but why 2**14?
  • Need @param and @type on WebSocketFrameDecoder.dataReceived
  • in _checkClientHandshake: "C{None} of not specified" ==> "C{None} if not specified"
  • In principle both of the XXXes in this file should reference tickets, but perhaps they're going away soon or will be rephrased as a design choice rather than an actual problem? If they stick around they need tickets.

• test_websocket.py
  • WebSocketResourceTests ==> WebSocketResourceTestCase
  • "we can register a handle for it using C{\}". Is this supposed to be a forward-slash?
  • In test_websocketProtocolAccepted: "The C{WebSocket-Procol} header" ==> "The C{WebSocket-Protocol} header"
  • In test_tooManyWebSocketProtocol: If several C{WebSocket-Procol} headers ==> If several C{WebSocket-Protocol} headers
  • in test_tooManyWebSocketProtocol, does "several" just mean more than one? If so might as well just be explicit about saying more than one.
  • WebSocketFrameDecoderTests ==> WebSocketFrameDecoderTestCase
  • in test_maxLength/test_maxLengthFrameCompleted it might be nice to use MAX_LENGTH to generate the oversized frame just in case that value changes.
  • WebSocketHandlerTests ==> WebSocketHandlerTestCase

• websocket.xhtml
  • It looks like this is still in flux, but is there a working protocol specification that can be linked to?
  • <code>Deferred/<code> => code>Deferred</code>. Other than that it lores fine.
  • if you want the text to link to API docs you need a class="API" attribute in the code tag, eg. <code class="API" base="twisted.web">websocket</code>
  • "The WebSocket specification specifies that frames must be encoded in UTF-8, but the implementation in Twisted doesn't make any check". Is there a reason to not implement the check?

I don't immediately have a way to test the WebSocket functionality from a browser, but I ran the examples in the howto documentation and in examples/ and they didn't have any other obvious failures.  Buildbot results look good.

[khorn]

FYI: the following link redirects to the most recent version of the draft:

 http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol

[jesstess]

The websocket.py example should also be listed with a blurb in web/examples/index.xhtml.

[thijs]

websocket.py also need an @since.

[therve]

Everything handled, I think, except:

A comment on why MAX_LENGTH is the number it is would be nice. It's a power of 2 but why 2**14?

This is a rather arbitrary value, afaict.

In principle both of the XXXes in this file should reference tickets, but perhaps they're going away soon or will be rephrased as a design choice rather than an actual problem? If they stick around they need tickets. 

Yes, I expect to remove them.

in test_maxLength/test_maxLengthFrameCompleted it might be nice to use MAX_LENGTH to generate the oversized frame just in case that value changes. 

The value is customized in setUp, so it should be fine?

"The WebSocket? specification specifies that frames must be encoded in UTF-8, but the implementation in Twisted doesn't make any check". Is there a reason to not implement the check?

Mainly because the specification is not clear about what to do (it lets you either ignore invalid characters or raise an error), but also because it's going to have a performance hit for correctly written applications.

Thanks a lot!

[therve]

Oh, and yes, I'm still working on the documentation. Any suggestion is welcome.

[thijs]

Moving it out of review if you're still working on the documentation..

[therve]

I really like code feedback though :). I'll move the documentation to a different ticket if it's a problem.

[feisley]

Update to correct branch

[feisley]

Tests pass and branch looks good.

Is the documentation complete at this point, or will that be split into its own ticket and this ticket be completed?

+1

[exarkun]

1. The test_emptyPostpath uses "/" as the request URI but it seems like it thinks it is testing the "" case?
2. A few test docstrings use C{} where I think they could probably use something else. I think C{} kind of means python expression. I'd use I{} for things like Origin or /. Up to you, I don't think it's really well defined.
3. The grammar in If more than one C{Origin} headers are present should be If more than one Origin header is present.
4. "http://localhost" when used as the value for the Origin header should probably be "http://localhost/" since the former isn't a legal URI/L. I guess it doesn't really matter, but it's nice to have valid data.
5. successfull should be successful
6. Watch out for docstrings with \x00 in them - that's a nul, not a representation of a nul. :)
7. # XXX maybe refuse fragments in URL - Real web servers don't pay attention to a buggy client that sends a fragment, so I guess there's no reason to do so here.
8. The originHeaders and hostHeaders checks in _checkClientHandshake seem redundant. It doesn't hurt though, but the checks in process should take care of these cases, perhaps?

I only got about half-way through the diff and now it's 11PM so...

[therve]

Thanks for the review!

[1] I removed the test, it was there for a previous implementation detail. [8] I actually need the value for those headers, whereas I can't get it in process. I think it's fine enough.

[mfenniak]

patch for client-side server connection error

[mfenniak]

Hi all,

I was experimenting with this WebSocket support (@28853) and found that whenever the socket was closed by the client, I would get this error:

2010-04-23 13:38:53-0600 [HTTPChannel,0,139.142.153.12] Unhandled error in Deferred: 2010-04-23 13:38:53-0600 [HTTPChannel,0,139.142.153.12] Unhandled Error

Traceback (most recent call last): Failure: twisted.internet.error.ConnectionDone: Connection was closed cleanly.

I've attached a patch (connectionlost.patch) that seems to handle this case more cleanly.

[TimAllen]

On Planet WebKit today I read:

Based on community feedback, the WebSocket specification has been updated to draft-ietf-hybi-thewebsocketprotocol-00 (also known as draft-hixie-thewebsocketprotocol-76).

This version relaxes requirements on handshake messages to make it easier to implement with HTTP libraries, and introduces nonce-based challenge-response to protect from cross protocol attacks. These changes make it incompatible with draft-hixie-thewebsocketprotocol-75; a client implementation of -75 can’t talk with a server implementation of -76, and vice versa.

I assume this patch is based on a previous version of the spec, and will need to be updated to match the new spec before it lands on the trunk?

[therve]

Probably. I think the branch is compatible with 75, so not with 76. I'm waiting to have a browser implementing this to know. I think the branch can still get some review though.

[exarkun]

(In [29175]) add a missing word

refs #4173

[exarkun]

1. The docs should probably say something about the reason parameter to connectionLost. It's pretty obvious to someone who's familiar with TCP and IProtocol already, but not to someone who's not.
2. In _checkClientHandshake:
   1. if not handlerFactory: would more correctly be if handlerFactory is None:
   2. if len(protocolHeaders) not in (0, 1): would be simpler as if len(protocolHeaders) > 1:. Or, the multiple checks against protocolHeaders could be probably flattened into an if/elif/else which is simpler than the current code.
3. There's an extra space on the first code line of renderWebSocket
4. WebSocketSite's docstring could probably say at least a little bit about the purpose of the class.
5. More generally:
   1. The UTF-8 verification point raised earlier might be made moot if write accepted a unicode instance instead of a str instance. This would clearly allow WebSocketTransport to enforce UTF-8-only. Likewise if the data were decoded using UTF-8 before being passed to frameReceived. That's not to say it would be a good idea to do this, but it's a possibility. It's too bad the specification gives up on this.
   2. Should write be named writeFrame or something else to disambiguate it from ITransport.write (having different names probably makes it easier to read and understand code using these APIs)?
   3. Should there (eventually?) be a way to send a large frame incrementally?
   4. Touching channel.transport and channel.setRawMode seems gross and fragile, but I still don't have anything better to suggest.

So, there's some stuff. I think the points are all pretty minor, still, except for the abstraction violation stuff at the end, which maybe we don't need to really worry about anyway.

What's the plan for merging this? Wait until the spec is finalized? Or merge it when it looks good, and track the spec as long as its changing?

[rlotun]

According to  the chromium blog:

"..starting from WebKit nightly build r59903 and Chrome 6.0.414.0 (r47952), the client will talk to a server using -76 version of the protocol, so it will fail to open WebSocket connections with a WebSocket server based on draft-hixie-thewebsocketprotocol-75"

[rlotun]

Support for the hixie-76 new handshake, with unit tests.

[rlotun]

I've added a patch to support either hixie-75 (Chrome 5 and Safari 5) or the latest hixie-76 (Chrome 6 dev channel). I've validated that the server works in all the aforementioned browsers. Unit tests have also been included.

[therve]

Thanks for the patch! Do you think we can drop support for pre-76 browsers? It sounds like there shouldn't be a lot of them.

[rlotun]

Replying to therve:

Thanks for the patch! Do you think we can drop support for pre-76 browsers? It sounds like there shouldn't be a lot of them.

The problem is that are major browsers working on 75 *now*. However, I think you're right in that those browsers who wanted to 'jump the gun' and implement a not-yet-finished standard would upgrade quickly (as seen in the dev-channel of Chrome).

I think it'd be reasonable for Twisted 10.1 to support only 76. One thing unknown to me is whether the standard will evolve any more - as in, perhaps this shouldn't be included in Twisted proper until the spec has been finalized.

As a side note, when implementation this patch (especially in light of the changes in hixie-76), I was wondering whether it was a good idea to subclasstwisted.web.server.Site. The WebSocket protocol bills itself as a new application-layer protocol that happens to use an HTTP-like handshake. Would it make more sense to implement it as a bare Protocol and server without subclassing any twisted.web stuff (but using its utilities for header-parsing)?

Thanks for taking a look at the patch.

[therve]

Replying to rlotun:

Replying to therve:

Thanks for the patch! Do you think we can drop support for pre-76 browsers? It sounds like there shouldn't be a lot of them.

The problem is that are major browsers working on 75 *now*. However, I think you're right in that those browsers who wanted to 'jump the gun' and implement a not-yet-finished standard would upgrade quickly (as seen in the dev-channel of Chrome).

AFAIK, only some versions of Chrome implements 75, and not the latest.

I think it'd be reasonable for Twisted 10.1 to support only 76. One thing unknown to me is whether the standard will evolve any more - as in, perhaps this shouldn't be included in Twisted proper until the spec has been finalized.

Yeah, the timeframe is going to be short for 10.1 anyway.

As a side note, when implementation this patch (especially in light of the changes in hixie-76), I was wondering whether it was a good idea to subclasstwisted.web.server.Site. The WebSocket protocol bills itself as a new application-layer protocol that happens to use an HTTP-like handshake. Would it make more sense to implement it as a bare Protocol and server without subclassing any twisted.web stuff (but using its utilities for header-parsing)?

So, I wish we could do that. First it's much easier to reuse the current API by subclassing, because of the way twisted.web is designed (IMHO). Then, even if it's a new application layer protocol, it ought to be integrated with a "normal" web application, serving a resource tree for example. Why do you think 76 change something in that regard?

[rlotun]

Replying to therve:

So, I wish we could do that. First it's much easier to reuse the current API by subclassing, because of the way twisted.web is designed (IMHO). Then, even if it's a new application layer protocol, it ought to be integrated with a "normal" web application, serving a resource tree for example. Why do you think 76 change something in that regard?

Fair enough - it was just a thought. With hixie-76 you need access to the body in the handshake part, and I was thinking the way I did it, using a transfer decoder, was a bit ugly (there could probably be a better way to do it).

I principle, it makes sense to be part of tweetdeck.web. I'll cede to your better judgement. Thanks.

[rlotun]

Replying to rlotun:

Replying to therve:

So, I wish we could do that. First it's much easier to reuse the current API by subclassing, because of the way twisted.web

I principle, it makes sense to be part of tweetdeck.web. I'll cede to your better judgement. Thanks.

Oops, I mean twisted.web ;-)

[ivank]

It would be nice to keep -75 support, because Safari 5 shipped with -75 support, and Safari seems to have an especially long upgrade cycle.

[progrium]

Before rlotun's patch was submitted, I took this module and started using it separately in my app. I found that I needed a) default/catchall handler support, and b) 75/76 support.

The default/catchall is a pretty minor change, but the new handshake led to a general refactoring of the handshake code. I modeled it roughly after em-websocket's implementation:  http://github.com/igrigorik/em-websocket/blob/master/lib/em-websocket/handler76.rb

I had a further constraint that it needed to run on Twisted 8 because we still use that in production, while I use Twisted 10 locally. This led to the discovery of a very simple solution for the capturing of the nonce/challenge key.

Anyway, I did this 76 implementation after I found the above patch, but ivank (I believe) suggested I post my version anyway. Perhaps it will be useful. I tested with actual browsers, but I'm pretty sure it will work against the tests in rlotun's patch.

[progrium]

Another constraint was that I needed to be able to subclass/mixin this WebSocketRequest with my own Request class that may not always be a WebSocket request. Hence the isWebSocket method, but this also was useful in the gotLength hack.

[progrium]

simpler implementation of 76 spec (works with older twisted as well), but also general refactoring

[BHSPitMonkey]

As discussed in IRC, there seems to be a bug when trying to call self.transport.write() inside of a WebsocketHandler's init method. The transport object is not ready at that point, and, as a result, Twisted sends a malformed response (HTTP 200) to the client. (Chromium doesn't like this!)

As a workaround for those who need to write to the transport in the constructor, use reactor.callLater(0, function, params).

[progrium]

So I just got a report that my patch has a very weird bug using the latest Chromium: it handshakes fine, keeps the connection open, but the packets sent from the browser just don't show up on the server. Nothing fails in the client, but nothing happens on the server. Strangely, rlotun's patch does work! But I spent all day trying to figure out what exactly would cause a difference between the two and I just can't find it.

For some reason, both Firefox and Safari are unable to load the example/websocket.py service, so I can't compare with their implementations. Anybody know why this is?

I did get all of rlotun's tests to pass, but still this bug persists, so I'm not posting an updated patch yet.

[phrearch]

cookie and connectionMade patch

[thijs]

Please add the 'review' keyword to this ticket if that patch should be reviewed.. See TwistedDevelopment.

[progrium]

Sorry, I'm new. Added review keyword, at least for mine: progrium-websocket-76.patch, described above.

[therve]

It's not ready for review yet.

[progrium]

That's not helpful.

[therve]

At least the latest patch doesn't contain any test. Also, the documentation needs to be updated accordingly.

[progrium]

Which documentation? Are you talking about for my patch?

[therve]

Replying to progrium:

Which documentation?

The websocket howto in the branch.

Are you talking about for my patch?

Nope. Apparently your patch doesn't work, so I'm not sure what to review?

[rlotun]

Replying to progrium:

Which documentation? Are you talking about for my patch?

Ok, so I'm confused. Progrium - you submitted a different patch but then you mentioned there was a bug with it? Can we perhaps merge our patches, test it with the latest browsers, add a few more unittests and then set the review keyword? Since this is a patch perhaps we can move this work off this ticket discussiont? I'm rlotun@…

[therve]

It would be great to hold the discussion on the mailing-list.

[rlotun]

Replying to therve:

It would be great to hold the discussion on the mailing-list.

Good idea. Done.

[glyph]

In accordance with that discussion, it appears that Reza has started developing this functionality  outside of Twisted, on Github, at least until the spec has settled down to the point where we can integrate it with Twisted's release cycles.

[MostAwesomeDude]

Adding myself to cc, as OSUOSL is interested in having this functionality inside Twisted.

One thing that appears to be occurring de facto is that people are using base64 encoding to get binary data across websockets. I cooked up a base64 wrapper for some internal stuff; that might possibly be something that should be included. (Or maybe the standard will stop sucking and will specify binary frames. I wouldn't hold my breath on that though.)

[alecm3]

Replying to therve:

Twisted should support the WebSocket protocol on the server side. See  http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol-16

It looks like the protocol has finally settled ( http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10 ), and the new protocol has been incorporated into Chrome 14 (available on Chrome dev channel now). The handshake was changed, and the message frames were changed substantially, compared to the older hixie-76 version, that people here were working on. The code to handle the new handshake is here:  http://svn.technocake.net/repos/dev/python/nettverk/websocket/myLibOnWebSock/websocketUtilities.py and the code to handle the new message frames is here:  http://pywebsocket.googlecode.com/svn/trunk/src/mod_pywebsocket/_stream_hybi.py

Are there any plans to include the new protocol into twisted?

[MostAwesomeDude]

Replying to alecm3:

Are there any plans to include the new protocol into twisted?

We (OSUOSL) would love to do so. There's a Twisted protocol for it ( http://paste.pocoo.org/show/451569/) that I have written, and that I'll be pushing up for review as soon as I add HyBi-10 support. I would add HyBi-06/07, but nobody appears to have actually shipped browsers using them, so I'll probably skip them.

However, I would say that there's no good indication that HyBi-10 is actually going to be stable beyond Chrome's decision to ship it. Fx might also ship it, but they are a ways off and would retarget to HyBi-11+ if such a thing occurred relatively soon.

If the spec freezes or if two major browser vendors ship it, I'll include it, but otherwise I feel that it is *still* premature.

(On a related note, the above code doesn't suppport Hixie-75 either.)

[alecm3]

Replying to MostAwesomeDude:

Replying to alecm3:

Are there any plans to include the new protocol into twisted?

We (OSUOSL) would love to do so. There's a Twisted protocol for it ( http://paste.pocoo.org/show/451569/) that I have written, and that I'll be pushing up for review as soon as I add HyBi-10 support. I would add HyBi-06/07, but nobody appears to have actually shipped browsers using them, so I'll probably skip them.

The reason I suspected it was stable was this message  http://www.ietf.org/mail-archive/web/hybi/current/msg07713.html : "what next: now that the WebSocket Protocol is stable, it is time to discuss the extension that we have put on hold for a while and decide on what we want to work."

[glyph]

Replying to alecm3:

The reason I suspected it was stable was this message  http://www.ietf.org/mail-archive/web/hybi/current/msg07713.html : "what next: now that the WebSocket Protocol is stable, it is time to discuss the extension that we have put on hold for a while and decide on what we want to work."

Sounds like wishful thinking to me: I'll believe it when I see it. That message still refers to several more anticipated discussions and at least one more expected draft spec. The first indication I would believe of some protocol stability would be an actual RFC number and no more "draft-" in the title. (Other draft specs I trust, but that's because they come from working groups that have a track record of making compatible recommendations.)

[wulczer]

I have a branch of Reza Lotun's txWebSocket on GitHub here:  https://github.com/wulczer/txWebSocket that supports the latest protocol (hybi-10) and has some small fixes as well.

See my pull requests:  https://github.com/rlotun/txWebSocket/pulls

[alecm3]

Replying to wulczer:

I have a branch of Reza Lotun's txWebSocket on GitHub here:  https://github.com/wulczer/txWebSocket that supports the latest protocol (hybi-10) and has some small fixes as well. See my pull requests:  https://github.com/rlotun/txWebSocket/pulls

Thanks Reza. Since you integrated this with the web server, I have two questions:

1. How do you set it up so that HTTP requests are served on a different port from WS requests?

2. How do you use this server for WS upgrade only, shutting down normal HTTP request capability (GET, POST, HEAD) all together?

[glyph]

This ticket should at least include a mention of Corbin Simpson's work, since he's been a bit more active lately  txWS, which I believe is a ground-up rewrite of txWebSockets and supports more protocols. I don't like a lot about relative merits.

Also,  Tavendo Autobahn, a websockets implementation with an undeniably shiny-looking website :). This one seems less interested in being integrated into Twisted but might be a more mature option for someone looking for a currently-supported solution. It certainly seems to have more in the way of documentation :).

[glyph]

Replying to glyph: Bah, hit send too soon.

I don't like a lot about relative merits.

I don't know a lot about their relative merits.

[wulczer]

Replying to alecm3:

Thanks Reza. Since you integrated this with the web server, I have two questions:

As I've been using Reza's implementation a lot, I think I can answer these questions:

1. How do you set it up so that HTTP requests are served on a different port from WS requests?

You can't do that with just WebSocketSite. But it's easy enough to have a normal twisted.web.server.Site running on one port and a WebSocketSite on another.

2. How do you use this server for WS upgrade only, shutting down normal HTTP request capability (GET, POST, HEAD) all together?

The WS upgrade starts with a GET request containing a Upgrade header, so you can't "shut down normal HTTP". What you probably want to do is something along the lines of:

wssite = websocket.WebSocketSite(resource.NoResource())
wssite.addHandler("/ws", handle_connection)

so that any HTTP request to your WS server will get a 404 reply, except ones requesting /ws with GET and the appropriate Upgrade header, which will follow the standard WebSocket upgrade process. Requests for /ws that do not contain an Upgrade header will also get a HTTP 404 reply.

By the way, you could also take Glyph's advice and evaluate other implementations, since txWebSocket, while perfectly functional and working for us in production, seems to be lacking in certain code quality aspects that the txWS rewrite will hopefully address.

[MostAwesomeDude]

I figured I should bump this gently.

txWS is a non-HTTP wrapper for WebSockets. That means that any HTTP request which is a real HTTP request and not just the faked kind used for WebSockets, will fail out hard, and by "hard" I mean that it won't get a real error or status code.

I think the big advantage of txWS is that it is compositional in how it wraps existing factories and protocols, and that's a big help in augmenting existing codebases. I don't know if that's an advantage for Twisted users at large, but considering that WebSockets are (in theory) a transport, it seems to make sense.

[alecm3]

As a heavy user of Twisted in production (we have 1.3Gbps bandwidth running 70+ Twisted TCP servers), we chose txWS over other implementations. We liked that it is lightweight and independent of the twisted web, and very simple: we just wrapped our existing Factory class in WebSocketFactory, and our new JavaScript client was ready to go: brilliantly simple, like Twisted was intended to be!

Replying to MostAwesomeDude:

I figured I should bump this gently. txWS is a non-HTTP wrapper for WebSockets. That means that any HTTP request which is a real HTTP request and not just the faked kind used for WebSockets, will fail out hard, and by "hard" I mean that it won't get a real error or status code. I think the big advantage of txWS is that it is compositional in how it wraps existing factories and protocols, and that's a big help in augmenting existing codebases. I don't know if that's an advantage for Twisted users at large, but considering that WebSockets are (in theory) a transport, it seems to make sense.

[iuridiniz]

I have a pull request over rlotun branch in order to allow make a old twisted protocols available over websocket through a classwrapper much like MostAwesomeDude did ( http://paste.pocoo.org/show/451569/)

 https://github.com/rlotun/txWebSocket/pull/16

[MostAwesomeDude]

Attached is a standalone patch which provides a new, hybrid approach to WebSockets, based on both txWebSockets and txWS. It comes with tests and implements RFC 6455.

[MostAwesomeDude]

#4173, with tests, hybrid approach

[habnabit]

(In [33693]) Branching to 'websocket-4173-3'

[habnabit]

This looks mostly good; we already talked offline about adding more test coverage.

1. The code that uses failed in WebSocketsResource.render should probably be refactored out into a separate function; it could be passed the request and return a pair of failed, key. Must setting Sec-WebSocket-Version on the response headers only happen if the request's version is invalid?
2. version is None or version != "13" can be just 'version != "13"'.
3. host and origin are assigned to but never used; pyflakes caught this.
4. protocol is checked against the encoders/decoders, but then gets shadowed by the protocol from self._factory.buildProtocol(...) and is never used. This apparently needs to be assigned to the codec attribute of the WebSocketsProtocol.
5. buildProtocol can return None to signal that the connection was rejected. This needs to be handled.
6. At least in the case of unknown opcodes, you might want to make an class UnknownOpcodeError(WSException) which gets raised with the opcode as one of the arguments, for easier introspection of _which_ unknown opcode it is.
7. PING frames are ignored? There doesn't appear to be any logic for responding to a PING with a PONG.

Thanks!

[MostAwesomeDude]

Replying to habnabit:

This looks mostly good; we already talked offline about adding more test coverage. 1. The code that uses failed in WebSocketsResource.render should probably be refactored out into a separate function; it could be passed the request and return a pair of failed, key. Must setting Sec-WebSocket-Version on the response headers only happen if the request's version is invalid?

The spec is not clear. Not in the least. I'd like to leave it here. Is this processing really that confusing? It's only a couple of clauses, but I could factor it out.

1. version is None or version != "13" can be just 'version != "13"'.

Done.

1. host and origin are assigned to but never used; pyflakes caught this.

Removed those. The RFC lifts the old requirement of servers to return Host and Origin headers, so we just won't check them. As far as host checking is concerned, that's the Site's duty.

1. protocol is checked against the encoders/decoders, but then gets shadowed by the protocol from self._factory.buildProtocol(...) and is never used. This apparently needs to be assigned to the codec attribute of the WebSocketsProtocol.

Renamed this to "codec" and wired it back up to the protocol, as we would want.

1. buildProtocol can return None to signal that the connection was rejected. This needs to be handled.

Talked it over with glyph, decided that 502 was a valid error status in this case. The buildProtocol() invocation is moved to before the request flush, and will set 502 on error.

1. At least in the case of unknown opcodes, you might want to make an class UnknownOpcodeError(WSException) which gets raised with the opcode as one of the arguments, for easier introspection of _which_ unknown opcode it is.

The opcode's included in the error message. I should point out that there are only the six opcodes listed in the RFC; any other opcode is *totally* unexpected.

1. PING frames are ignored? There doesn't appear to be any logic for responding to a PING with a PONG.

Bizarre. Well, it's in there now. We don't send any PINGs ourselves though.

Thanks!

No, no, thank *you*.

[MostAwesomeDude]

Next iteration of patch, after habnabit's review

[habnabit]

Okay, this looks a lot better, and I've been further reassured after talking to you offline. The refactoring of render definitely isn't necessary if you don't think it would be useful. Just a few little things:

1. I would make some sort of remark about the Sec-WebSocket-Protocol and not caring about if there's a comma-separated list.
2. I would also make a reverse dictionary from opcode_types and use that as e.g. make_hybi07_frame(reason, opcode=opcodes[CLOSE]) instead of a few scattered (and duplicated!) magic numbers.
3. close should be renamed loseConnection, so that calling self.transport.loseConnection() will send the CLOSE frame.

[MostAwesomeDude]

Replying to habnabit:

Okay, this looks a lot better, and I've been further reassured after talking to you offline. The refactoring of render definitely isn't necessary if you don't think it would be useful. Just a few little things: 1. I would make some sort of remark about the Sec-WebSocket-Protocol and not caring about if there's a comma-separated list. 1. I would also make a reverse dictionary from opcode_types and use that as e.g. make_hybi07_frame(reason, opcode=opcodes[CLOSE]) instead of a few scattered (and duplicated!) magic numbers. 1. close should be renamed loseConnection, so that calling self.transport.loseConnection() will send the CLOSE frame.

More comments about the protocol/codec system have been added.

There's now a reversed symbolic mapping for opcodes. The magic numbers have been consolidated.

As we discussed in-person, we are now uniformly sending close packets when losing the connection, unless the connection was already lost.

Also, there are some more misc. comments, and the log notes both the start and end of connections.

[MostAwesomeDude]

Yet another version of #4173.

[MostAwesomeDude]

I'm now a committer! Yay! So the latest patch has been pushed onto the ticket branch; review it there, please. Thanks.

[MostAwesomeDude]

(In [33775]) Add a short howto and example for WS.

Refs #4173

[MostAwesomeDude]

(In [33778]) Add howto for WebSockets.

Stupid SVN. This better go to the right branch this time.

Refs #4173

[dreid]

Thanks for working on this MostAwesomeDude. I'm pretty sure it'll be some sort of record if we can get a feature like this in a release in under 3 years.

• Doesn't follow spacing standard. 3 spaces between top level classes and functions, 2 spaces between methods. You can use  https://github.com/cyli/TwistySublime/blob/master/twisted_pep8.py to check for these automatically.

  (twisted)beckett dreid:trunk> svn status | grep '^M|^A' | grep ".*py$" | cut -c 8- | xargs python ~/Library/Application\ Support/Sublime\ Text\ 2/Packages/TwistySublime/twisted_pep8.py
  twisted/web/test/test_websockets.py:18:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:27:1: E302 expected 3 blank lines, found 1
  twisted/web/test/test_websockets.py:38:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:42:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:51:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:59:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:70:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:81:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:95:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:106:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:117:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:129:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:142:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:148:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:154:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:160:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:166:5: E301 expected 2 blank lines, found 1
  twisted/web/test/test_websockets.py:172:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:78:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:92:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:112:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:202:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:215:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:219:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:254:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:267:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:280:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:290:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:300:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:320:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:331:1: E302 expected 3 blank lines, found 1
  twisted/web/websockets.py:349:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:352:5: E301 expected 2 blank lines, found 1
  twisted/web/websockets.py:355:5: E301 expected 2 blank lines, found 1
  

• Test docstrings shouldn't begin with "Test"
• TestHyBi07Helpers has lots of test_ methods that don't include docstrings.
  • Additionally some of the docstrings simply reference sections of the RFC and what I assume are pre-RFC drafts. These should be expanded to describe what they're actually testing in plain english. If you're going to reference RFCs then it might be a good idea to include URLs to the RFC and drafts in the Test class docstring.
• self.assertEqual(frames, []) or self.assertEqual(len(frames), 0) might be preferable to self.assertFalse(frames)
• WSException docstring references txWS and has a confusing comment about "escaping" txWS. I assume this means that application code should never see it so I'd recommend _WSException instead.
• Type enums, opcode_types, opcode_for_types, encoders, and decoders look private.
• Perhaps the guid in make_accept should be a private module level constant?
• It looks like WebSocketsResource is the only public API in this module, the frame helpers and the protocol and factory should probably be private.
• All frame helper functions should use epytext to document parameters and return values.
• WebSocketsProtocol, WebSocketsFactory, and WebSocketsResource don't have any test coverage.
• Needs a News fragment as described in http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles
• Code fragments in the howto should reference files in the listings directory instead of inlined. Usage looks like this (from templates howto):

  <a href="listings/template-1.xml" class="html-listing">template example</a>
  

• WebSocketsResource should include @since in it's docstring. Like @since 12.1

Thanks again for working on this. Do we support websockets is a really common question and it'll be nice to be able to point to the howto after this lands.

Keep up the good work and let me know if any of these points are unclear.

[MostAwesomeDude]

(In [34770]) web/websockets: Respace to comply with coding standard.

Refs #4173

[MostAwesomeDude]

(In [34771]) web/test/test_websockets: Redo docstrings.

Refs #4173

[MostAwesomeDude]

(In [34777]) web/websockets: Assorted cleanups.

Make a bunch of things private, add more docstrings, clean up tests, etc.

Refs #4173

[MostAwesomeDude]

(In [34781]) web/websockets: More docstrings and privates.

Refs #4173

[MostAwesomeDude]

Okay, I got most of the way through this before our #3204-related hax bit me. I'll hack the rest of this out tomorrow, but for now, this is looking good.

[oberstet]

Who will call startFactory/stopFactory for a factory wrapped and added as a Resource to Site via WebSocketsResource?

On first look, it seems to make sense that the Site factory within it's start/stopFactory calls start/stopFactory on all factories which are wrapped as Resources and added to that site.

[lvh]

This currently imports NoResource from t.w.error:

from twisted.web.error import NoResource

Since the latest release of twisted, NoResource is no longer available there and should be imported from twisted.web.resource.

[tekNico]

Enable TLS encryption on websockets (by tekNico and frankban)

[tekNico]

Ehi, MostAwesomeDude, I hope you didn't throw in the towel here yet. :-)

Look at the attached websockets-tls.diff file to enable TLS encryption on websockets (courtesy of Francesco 'frankban' Banconi and myself, it took a while to figure it out. :-) )

Thanks for your work!

[MostAwesomeDude]

Is that TLS patch correct? I thought that, in a TLS situation, the entire connection would be TLS'd, including the initial HTTP headers. So the connection would already have TLS and no additional TLS would be needed.

[tekNico]

Yes, the initial connection needs to be already encrypted (HTTPS). Then the connection is upgraded to WebSocket, and the encrypting wrapper protocol needs to be retained in order to wrap the WebSocket protocol, so the same protocol encrypts both the initial HTTP and the following WebSocket communications on the same connection.

[aprilmay]

Use long words for xor masking

[aprilmay]

We can burn quite a bit of CPU for masking/unmasking possibly large amount of data, so i think it makes sense to optimize that part, at the cost of slightly obscure code :)

The patch uses the CPU word length (64, 32 bits). I tested it quite a bit, it works well with speedups from 3 to 6 times from the original function.

Can you please review the patch just submitted?

[glyph]

Replying to aprilmay:

We can burn quite a bit of CPU for masking/unmasking possibly large amount of data, so i think it makes sense to optimize that part, at the cost of slightly obscure code :) The patch uses the CPU word length (64, 32 bits). I tested it quite a bit, it works well with speedups from 3 to 6 times from the original function. Can you please review the patch just submitted?

Thanks for submitting this.

The first step in dealing with performance issues like this is to develop a repeatable benchmark; ideally something we can run on our  benchmark site.

As you've noted, the code is somewhat obscure, so this could probably be landed as a separate ticket after the main implementation has gotten to trunk.

Finally, we prefer patches in unified diff format. This, and other guidelines for contribution, can be found at the wiki page on contributing.

[vic_in_kh]

How about handling cookies?

[vic_in_kh]

add onRequest handler where is possible to read cookies

[therve]

OK, I think this is ready for another round of review. I completed I think test coverage, and did a bunch of cleanups (coding standard, old code, renaming). I put the HTTPS fix in it, but not the performance enhancement of mask/unmask.

Build is here:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/websocket-4173-3

[kapilt]

one additional thing we've needed with this branch code in practice for chrome is this two line codec conditional for 'undefined' in render.

codec = request.getHeader("Sec-WebSocket-Protocol")

if codec == 'undefined':

codec = None

if codec:

if codec not in _encoders or codec not in _decoders:

log.msg("Codec %s is not implemented" % codec)

[tom.prince]

1. howto
   1. Is there any reason not to simply do
      Error: Failed to load processor javascript

      No macro or processor named 'javascript' found

instead of

Error: Failed to load processor javascript

No macro or processor named 'javascript' found

?

1. The example should either use react, or probably better twistd.
2. I tried the example (before looking at the howto), and tried to go to http://localhost:7080/' and just got NoResource`. I'd expect that to go to the demo instead.
   • And after reading the howto, I'm no more informed.
   • In any case, since the html is being served by the server in the howto, I think it would make more sense to connect to /ws on server, so somebody playing with the example doesn't need to change both the python and html, to test it out on a different port or machine.
3. It seems redundant to say that WebSocketsResource is a Resource.
4. This doesn't seem to explain what is going on.

   The following example creates a <code>Site</code> which responds to all
   requests with a WebSockets connection, which serves an
   <code>EchoProtocol</code>.
   

   Rather, requests to /ws get a websocket connection which serves an EchoProtocol.
5. Should we mention that a modern browser is needed to make the html into a websocket client? Or maybe this could be worded in such a way that it doesn't use the word client?
6. Should the howto mention browser support at all, and supported versions of the websocket RFC (or at least pointer to the same)?

1. The change in twisted/web/http.py should update the comment and/or add a new constant instead of NO_BODY_CODES.
2. I guess perhaps twisted.python.constants.Values isn't flexible enough to handle _opcodeTypes and _opcodeForType, which is sad. (File a ticket for handling multivalued constants, that have a canonical value maybe?)

Here are some initial thoughts. I'll continue this review after lunch.

[tom.prince]

trac fail:

websocket.onopen = onOpen;

instead of

websocket.onopen = function(evt) { onOpen(evt) };

[tom.prince]

I just spent some time reading the current RFC, and have some comments on support for bits of the protocol that would have api implications.

1. The RFC suggests that the websocket protocol is datagram protocol, rather than a stream protocol. The client api  WSAPI supports this interpretation. The protocol also specifies two type of frames (text and binary) and requires that text frames be UTF-8 encoded (and that the encoding be checked).
2. The code suggest that Sec-WebSocket-Protocol should really be called codec. However the RFC gives an example

   Sec-WebSocket-Protocol: chat, superchat
   

   which suggests that it really is about supporting different protocols at a single endpoint (hybi-7 and hybi-10 have the same example). The IANA  subprotocol registry also suggests this. Thus I think the current use of this for a codec should probably be removed, and at some point, support for multiple protocols should be added (this could be in a separate ticket, but the API implications should be considered in this ticket).
3. I don't see anything to suggest that a websocket endpoint (resource name in the RFC) needs to be a leaf resource. I also don't see anything suggesting that a given endpoint only accept websocket connections. That is, I think a given resource name could respond to both websocket and http requests.
4. The RFC suggests that the websocket endpoint may want to examine additional headers (in particular cookies and the origin header) as well as the query string.
5. There is no support for sending PINGs or timing out a connection of there isn't timely PONGs.
6. There is no provision on how to handle extensions. This could reasonably wait until some extension exists, though.

I think the above considerations may have enough api impact that a detailed code review at this point may be premature.

[tom.prince]

Some additional thoughts

1. While websockets are datagram based, it supports protocol level fragmentation, and the RFC seems to implicitly suggest allowing large datagrams to be handled in a streaming fashion.
2. The RFC suggests as possible extensions allowing out-of-order or interleaved datagrams, as well as multiplexing.

[therve]

(In [37149]) Remove codec functionality, add a way to specify the protocol via a resource method

Refs #4173

[therve]

(In [37150]) Make an interface to highlight the new method

Refs #4173

[oberstet]

A few comments/opinions ..

Replying to tom.prince:

I just spent some time reading the current RFC, and have some comments on support for bits of the protocol that would have api implications. 1. The RFC suggests that the websocket protocol is datagram protocol, rather than a stream protocol. The client api  WSAPI supports this interpretation. The protocol also specifies two type of frames (text and binary) and requires that text frames be UTF-8 encoded (and that the encoding be checked).

The client API exposed is orthogonal to the protocol. While the JavaScript WebSocket API exposed in browsers is (currently) message based, there are implementations of WebSocket that expose (also) a frame-based and streaming API (ie Autobahn,  http://autobahn.ws/python/tutorials/streaming and  http://autobahn.ws/python/tutorials/producerconsumer). Regarding frame types: WebSocket extensions may specify additional frame types.

Regarding UTF8 checking and general protocol compliance, there is  http://autobahn.ws/testsuite (Disclosure: I am author of that stuff).

1. The code suggest that Sec-WebSocket-Protocol should really be called codec. However the RFC gives an example {{{ Sec-WebSocket-Protocol: chat, superchat }}} which suggests that it really is about supporting different protocols at a single endpoint (hybi-7 and hybi-10 have the same example). The IANA  subprotocol registry also suggests this. Thus I think the current use of this for a codec should probably be removed, and at some point, support for multiple protocols should be added (this could be in a separate ticket, but the API implications should be considered in this ticket).

There are 2 dimensions where WebSocket can be extended: "WebSocket extensions" and "WebSocket subprotocols". The latter is for application-level protocols. Hence, there needs to be API for apps.

For example, the JS browser API lets you specify which subprotocol(s) the client speaks when opening a WebSocket connection. Apart from that, a WebSocket implementation should be "agnostic" to subprotocols. I.e. one might develop a Twisted perspective broker binding to WebSocket, in which case the spoken WebSocket subprotocol might be "txpb". But "txpb" would be implemented _above_ the raw WebSocket implementation.

1. I don't see anything to suggest that a websocket endpoint (resource name in the RFC) needs to be a leaf resource. I also don't see anything suggesting that a given endpoint only accept websocket connections. That is, I think a given resource name could respond to both websocket and http requests.

Yes, right. The spec even allows for a client to speak plain old HTTP request/request with a resource (URL) initially, and then at some point upgrade to speak WebSocket to that same resource. However, after the upgrade was successfully finished, there is no way to "downgrade" again. Should the upgrade fail, the client may continue to speak plain HTTP.

In practice, my guess would be: its fine to have a dedicated leaf resource for WebSocket access.

1. The RFC suggests that the websocket endpoint may want to examine additional headers (in particular cookies and the origin header) as well as the query string.

Yep. However, fragments in URLs are not allowed with WebSocket.

1. There is no support for sending PINGs or timing out a connection of there isn't timely PONGs.

The spec says MUST reply to PING, and SHOULD do so as soon as practical, and MAY only reply to the last received PING if it hasn't already replied (which is what IE does for example). Formally, a client never responding is valid .. since it may just deem replying not yet practical and wait for another PING to reply to. The spec does not talk about timing out a connection when not receiving PONGs. Mechnanisms for that are either app business or implementation specific.

In practice, most implementations just reply immediately to each PING with a PONG.

1. There is no provision on how to handle extensions. This could reasonably wait until some extension exists, though.

As far as I can tell, there are 2 extensions in the cooking currently: per-message compression and MUX.

I think the above considerations may have enough api impact that a detailed code review at this point may be premature.

[tom.prince]

1. The RFC suggests that the websocket protocol is datagram protocol, rather than a stream protocol. The client api WSAPI supports this interpretation. The protocol also specifies two type of frames (text and binary) and requires that text frames be UTF-8 encoded (and that the encoding be checked).

The client API exposed is orthogonal to the protocol. While the JavaScript WebSocket API exposed in browsers is (currently) message based, there are implementations of WebSocket that expose (also) a frame-based and streaming API (ie Autobahn,  http://autobahn.ws/python/tutorials/streaming and  http://autobahn.ws/python/tutorials/producerconsumer). Regarding frame types: WebSocket extensions may specify additional frame types.

Sure, I don't have a problem with exposing the lower-level details. The current implementation here, however, *only* exposes frames, and doesn't expose message boundaries or different frame-types.

On a related note, this part of the protocol seems reminicent of  SCTP ( RFC 4960), so it might be worthwhile considering a common interface.

Yep. However, fragments in URLs are not allowed with WebSocket.

You shouldn't see them in http requests either.

5. There is no support for sending PINGs or timing out a connection of there isn't timely PONGs.

The spec says MUST reply to PING, and SHOULD do so as soon as practical, and MAY only reply to the last received PING if it hasn't already replied (which is what IE does for example). Formally, a client never responding is valid .. since it may just deem replying not yet practical and wait for another PING to reply to. The spec does not talk about timing out a connection when not receiving PONGs. Mechnanisms for that are either app business or implementation specific.

Well, the current implementation doesn't seem to ever send a ping. Certainly, timeouts are going to be application specific, but the current code doesn't provide any way for application to implement timeouts.

[therve]

Note that I don't think we really care about having a pure RFC compliant implementation. In particular in real application, right now, binary/uft-8 distinction doesn't matter, so I think what the branch is doing is good. Same for the datagram versus stream point, we don't care about that, because as far as the app goes it is a connected protocol.

The point about Sec-WebSocket-Protocol and PING are valid, I'm going to address those.

And please don't mention SCTP ever again :).

[lvh]

FWIW, I agree completely with therve here, if I understand correctly there is not even an *unreasonable* way to actually build things in browsers with the binary protocol, let alone a reasonable way. (That is, you can get binary strings in the browser, and then you can do prretty much nothing at all with them -- when I wanted to get bidirectional AMP-with-browsers, I ended up concocting a JSON serialization for AMP rather than trying to do actual AMP; and AMP is hardly the most complicated of binary protocols.)

[Slavon]

 Neti pot

[davep]

In the docstring for the implementation of lookupProtocol, the second sentence needs some work. I would suggest:

This default implementation ignores the protocol names and just returns a protocol built by C{self._factory}.

Why is the method returning a tuple if the second element is always None?

[davep]

On line 460 I think 'twistd' needs to be 'twisted'.

[davep]

Replying to davep:

On line 460 I think 'twistd' needs to be 'twisted'.

Also 386 and 443.

[davep]

The init for WebSocketsResource needs a docstring.

[davep]

On line 420, 'WebsocketsResourceTest' should be 'WebSocketsResource'.

[davep]

On line 462, I think you want C{str} instead of strinf.

Sorry for the disjointed feedback, I wasn't reading to review, just for my own understanding, but then I kept noticing stuff :)

[therve]

Thanks for the feedback (mostly handled in [37548]), but yeah please fit it in one comment if possible :).

[davep]

Ok, actual organization this time:

1. Line 60 -- do you want this sort of editorial in a docstring?

2. Lines 142 & 143 - change needMast to needMask

3. Change two instances of "we're gonna" to "we will", lines 180 and 533.

4. Line 451 - "matched protocol name or None"

5. Line 477 - docstring on init

6. Line 488 - change WebsocketsResource to WebSocketsResource (capital S)

7. Line 504 - "for the given protocol names", "This default implementation

ignores the protocol names and just returns a protocol instance built by {self._factory}.

[therve]

(In [37553]) Fix some docstrings and comments

Refs #4173

[davep]

A few more observations:

1. I guess lookupProtocol is there because you might need to choose

the protocol based on the headers from the client, and the default factory wouldn't be given that information? It might be good to document why you would want to override lookupProtocol. Is 'lookup' the right word, since lookupProtocol is supposed to return a brand new protocol instance every time?

2. Line 437 -- change "is used as" to "is used as the"

3. Line 501 -- change "return" to "returns"

4. In test_websockets.py, SavingEchoFactory doesn't appear to be

used for anything.

[davep]

In my testing I notice that when sending the headers, the Request will set the Transfer-Encoding to 'chunked' since there is no Content-Length. It doesn't seem to bother Chrome, but isn't it a bit weird, since no data is ever actually sent in chunked form?

[dav01]

Hi, looking forward to this. Good work everybody. Sorry to hijack the ticket thread for this but who should I talk to for roadmap and latest news for the WS-integration.