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.