Opened 5 years ago

Last modified 7 months ago

#4173 enhancement new

WebSocket server support

Reported by: therve Owned by: therve
Priority: normal Milestone:
Component: web Keywords:
Cc: jesstess, thijs, progrium@…, MostAwesomeDude@…, alecm3, tom.sheffler@…, gmludo@…, lvh, narthollis+twisted@…, twisted@…, tobias.oberstein@…, frankban@…, aprilmay, dynamicgl@…, vic@…, dave@…, daf@…, free@…, burak+twisted@…, chance.zibolski@… Branch: branches/websocket-4173-4
(diff, github, buildbot, log)
Author: therve, MostAwesomeDude, habnabit Launchpad Bug:

Description (last modified by therve)

Twisted should support the WebSocket protocol on the server side.

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

Attachments (11)

connectionlost.patch (1.0 KB) - added by mfenniak 5 years ago.
patch for client-side server connection error
rlotun-websocket-76.patch (10.1 KB) - added by rlotun 5 years ago.
Support for the hixie-76 new handshake, with unit tests.
progrium-websocket-76.patch (8.1 KB) - added by progrium 4 years ago.
simpler implementation of 76 spec (works with older twisted as well), but also general refactoring
cookie_connectionMade.patch (1.1 KB) - added by phrearch 4 years ago.
cookie and connectionMade patch
4173-3.patch (18.7 KB) - added by MostAwesomeDude 3 years ago.
#4173, with tests, hybrid approach
4173-4.patch (19.5 KB) - added by MostAwesomeDude 3 years ago.
Next iteration of patch, after habnabit's review
4173-5.patch (21.2 KB) - added by MostAwesomeDude 3 years ago.
Yet another version of #4173.
websockets-tls.diff (615 bytes) - added by tekNico 2 years ago.
Enable TLS encryption on websockets (by tekNico and frankban)
fast_xor.diff (1.1 KB) - added by aprilmay 2 years ago.
Use long words for xor masking
request_handler.patch (458 bytes) - added by vic_in_kh 2 years ago.
add onRequest handler where is possible to read cookies
amp-example.tar.gz (40.0 KB) - added by tom.prince 18 months ago.
Example binary amp from browser.

Download all attachments as: .zip

Change History (159)

comment:1 Changed 5 years ago by therve

  • Description modified (diff)

comment:2 Changed 5 years ago by therve

  • Author set to therve
  • Branch set to branches/websocket-4173

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

comment:4 Changed 5 years ago by therve

  • Keywords review added
  • Owner therve deleted

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!

comment:5 Changed 5 years ago by jesstess

  • Cc jesstess added
  • Owner set to jesstess

comment:6 Changed 5 years ago by jesstess

  • Keywords review removed
  • Owner changed from jesstess to therve

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 214?
    • 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.

comment:7 Changed 5 years ago by khorn

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

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

comment:8 Changed 5 years ago by jesstess

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

comment:9 Changed 5 years ago by thijs

  • Cc thijs added

websocket.py also need an @since.

comment:10 Changed 5 years ago by therve

  • Keywords review added
  • Owner therve deleted

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!

comment:11 Changed 5 years ago by therve

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

comment:12 Changed 5 years ago by thijs

  • Keywords review removed
  • Owner set to therve

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

comment:13 Changed 5 years ago by therve

  • Keywords review added
  • Owner therve deleted

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

comment:14 Changed 5 years ago by feisley

  • Branch changed from branches/websocket-4173 to branches/websocket-4173-2

Update to correct branch

comment:15 Changed 5 years ago by feisley

  • Owner set to feisley

comment:16 Changed 5 years ago by feisley

  • Keywords review removed
  • Owner changed from feisley to therve

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

comment:17 Changed 5 years ago by feisley

  • Keywords review added

comment:18 Changed 5 years ago by exarkun

  • Keywords review removed
  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...

comment:19 Changed 5 years ago by therve

  • Branch changed from branches/websocket-4173-2 to branches/websocket-4173
  • Keywords review added

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.

comment:20 Changed 5 years ago by therve

  • Owner therve deleted

comment:21 Changed 5 years ago by therve

  • Branch changed from branches/websocket-4173 to branches/websocket-4173-2

Changed 5 years ago by mfenniak

patch for client-side server connection error

comment:22 Changed 5 years ago by 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.

comment:23 Changed 5 years ago by 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?

comment:24 Changed 5 years ago by 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.

comment:25 Changed 5 years ago by exarkun

(In [29175]) add a missing word

refs #4173

comment:26 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  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?

comment:27 Changed 5 years ago by 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"

Changed 5 years ago by rlotun

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

comment:28 Changed 5 years ago by 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.

comment:29 follow-up: Changed 5 years ago by 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.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 years ago by 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.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by 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?

comment:32 in reply to: ↑ 31 ; follow-up: Changed 5 years ago by 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.

comment:33 in reply to: ↑ 32 Changed 5 years ago by 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 ;-)

comment:34 Changed 4 years ago by 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.

comment:35 Changed 4 years ago by 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.

comment:36 Changed 4 years ago by 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.

comment:37 Changed 4 years ago by progrium

  • Cc progrium@… added

Changed 4 years ago by progrium

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

comment:38 Changed 4 years ago by 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).

comment:39 Changed 4 years ago by 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.

Changed 4 years ago by phrearch

cookie and connectionMade patch

comment:40 Changed 4 years ago by thijs

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

comment:41 Changed 4 years ago by progrium

  • Keywords review added
  • Owner therve deleted

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

comment:42 Changed 4 years ago by therve

  • Keywords review removed
  • Owner set to therve

It's not ready for review yet.

comment:43 Changed 4 years ago by progrium

That's not helpful.

comment:44 Changed 4 years ago by therve

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

comment:45 follow-ups: Changed 4 years ago by progrium

Which documentation? Are you talking about for my patch?

comment:46 in reply to: ↑ 45 Changed 4 years ago by 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?

comment:47 in reply to: ↑ 45 Changed 4 years ago by 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@…

comment:48 follow-up: Changed 4 years ago by therve

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

comment:49 in reply to: ↑ 48 Changed 4 years ago by rlotun

Replying to therve:

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

Good idea. Done.

comment:50 Changed 4 years ago by 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.

comment:51 Changed 4 years ago by <automation>

  • Owner therve deleted

comment:52 Changed 4 years ago by MostAwesomeDude

  • Cc MostAwesomeDude@… added

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.)

comment:53 in reply to: ↑ description ; follow-up: Changed 3 years ago by alecm3

  • Cc alecm3 added

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?

comment:54 in reply to: ↑ 53 ; follow-up: Changed 3 years ago by 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.)

comment:55 in reply to: ↑ 54 ; follow-up: Changed 3 years ago by 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."

comment:56 in reply to: ↑ 55 Changed 3 years ago by 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.)

comment:57 follow-up: Changed 3 years ago by 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

comment:58 in reply to: ↑ 57 ; follow-up: Changed 3 years ago by 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?
  1. How do you use this server for WS upgrade only, shutting down normal HTTP request capability (GET, POST, HEAD) all together?

comment:59 follow-up: Changed 3 years ago by 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 :).

comment:60 in reply to: ↑ 59 Changed 3 years ago by 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.

comment:61 in reply to: ↑ 58 Changed 3 years ago by 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.

  1. 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.

comment:62 follow-up: Changed 3 years ago by 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.

comment:63 in reply to: ↑ 62 Changed 3 years ago by 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.

comment:64 Changed 3 years ago by 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

comment:65 Changed 3 years ago by MostAwesomeDude

  • Author changed from therve to therve, MostAwesomeDude
  • Keywords review added

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.

Changed 3 years ago by MostAwesomeDude

#4173, with tests, hybrid approach

comment:66 Changed 3 years ago by habnabit

  • Owner set to habnabit

comment:67 Changed 3 years ago by habnabit

  • Author changed from therve, MostAwesomeDude to therve, MostAwesomeDude, habnabit
  • Branch changed from branches/websocket-4173-2 to branches/websocket-4173-3

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

comment:68 follow-up: Changed 3 years ago by habnabit

  • Keywords review removed
  • Owner changed from habnabit to MostAwesomeDude

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!

comment:69 in reply to: ↑ 68 Changed 3 years ago by MostAwesomeDude

  • Keywords review added

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*.

Changed 3 years ago by MostAwesomeDude

Next iteration of patch, after habnabit's review

comment:70 follow-up: Changed 3 years ago by habnabit

  • Keywords review removed

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.

comment:71 in reply to: ↑ 70 Changed 3 years ago by MostAwesomeDude

  • Keywords review added

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.
  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.

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.

Changed 3 years ago by MostAwesomeDude

Yet another version of #4173.

comment:72 Changed 3 years ago by thijs

  • Owner MostAwesomeDude deleted

comment:73 Changed 3 years ago by MostAwesomeDude

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

comment:74 Changed 3 years ago by MostAwesomeDude

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

Refs #4173

comment:75 Changed 3 years ago by MostAwesomeDude

(In [33778]) Add howto for WebSockets.

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

Refs #4173

comment:76 Changed 3 years ago by dreid

  • Keywords review removed
  • Owner set to MostAwesomeDude

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.

comment:77 Changed 3 years ago by tomsheffler

  • Cc tom.sheffler@… added

comment:78 Changed 3 years ago by GMLudo

  • Cc gmludo@… added

comment:79 Changed 3 years ago by lvh

  • Cc lvh added

comment:80 Changed 3 years ago by narthollis

  • Cc narthollis+twisted@… added

comment:81 Changed 3 years ago by dantman

  • Cc twisted@… added

comment:82 Changed 2 years ago by MostAwesomeDude

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

Refs #4173

comment:83 Changed 2 years ago by MostAwesomeDude

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

Refs #4173

comment:84 Changed 2 years ago by MostAwesomeDude

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

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

Refs #4173

comment:85 Changed 2 years ago by MostAwesomeDude

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

Refs #4173

comment:86 Changed 2 years ago by 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.

comment:87 Changed 2 years ago by oberstet

  • Cc tobias.oberstein@… added

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.

comment:88 Changed 2 years ago by 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.

Changed 2 years ago by tekNico

Enable TLS encryption on websockets (by tekNico and frankban)

comment:89 Changed 2 years ago by 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!

comment:90 Changed 2 years ago by tekNico

  • Cc tekNico added

comment:91 Changed 2 years ago by frankban

  • Cc frankban@… added

comment:92 Changed 2 years ago by 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.

comment:93 Changed 2 years ago by 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.

comment:94 Changed 2 years ago by aprilmay

  • Cc aprilmay added

comment:95 Changed 2 years ago by dynamicgl

  • Cc dynamicgl@… added

Changed 2 years ago by aprilmay

Use long words for xor masking

comment:96 follow-up: Changed 2 years ago by 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?

comment:97 in reply to: ↑ 96 Changed 2 years ago by 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.

comment:98 in reply to: ↑ description Changed 2 years ago by vic_in_kh

  • Cc vic@… added

How about handling cookies?

Changed 2 years ago by vic_in_kh

add onRequest handler where is possible to read cookies

comment:99 Changed 2 years ago by therve

  • Keywords review added
  • Owner MostAwesomeDude deleted

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

comment:100 Changed 23 months ago by 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)

comment:101 Changed 22 months ago by tom.prince

  • Owner set to tom.prince

comment:102 Changed 22 months ago by tom.prince

  1. howto
    1. Is there any reason not to simply do
      websocket.onopen = onOpen
      

instead of

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

?

  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.

comment:103 Changed 22 months ago by tom.prince

trac fail:

websocket.onopen = onOpen;

instead of

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

comment:104 follow-up: Changed 22 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to therve

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.

comment:105 Changed 22 months ago by 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.

comment:106 Changed 22 months ago by therve

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

Refs #4173

comment:107 Changed 22 months ago by therve

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

Refs #4173

comment:108 in reply to: ↑ 104 Changed 22 months ago by 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.

comment:109 Changed 22 months ago by 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.

  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.

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.

comment:110 Changed 22 months ago by 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 :).

comment:111 follow-up: Changed 22 months ago by 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.)

comment:112 Changed 21 months ago by davep

  • Cc dave@… added

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?

comment:113 follow-up: Changed 21 months ago by davep

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

comment:114 in reply to: ↑ 113 Changed 21 months ago by davep

Replying to davep:

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

Also 386 and 443.

comment:115 Changed 21 months ago by davep

The init for WebSocketsResource needs a docstring.

comment:116 Changed 21 months ago by davep

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

comment:117 Changed 21 months ago by 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 :)

comment:118 Changed 21 months ago by therve

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

comment:119 Changed 21 months ago by davep

Ok, actual organization this time:

  1. Line 60 -- do you want this sort of editorial in a docstring?
  1. Lines 142 & 143 - change needMast to needMask
  1. Change two instances of "we're gonna" to "we will", lines 180 and 533.
  1. Line 451 - "matched protocol name or None"
  1. Line 477 - docstring on init
  1. Line 488 - change WebsocketsResource to WebSocketsResource (capital S)
  1. Line 504 - "for the given protocol names", "This default implementation

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

comment:120 Changed 21 months ago by therve

(In [37553]) Fix some docstrings and comments

Refs #4173

comment:121 Changed 21 months ago by 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?
  1. Line 437 -- change "is used as" to "is used as the"
  1. Line 501 -- change "return" to "returns"
  1. In test_websockets.py, SavingEchoFactory doesn't appear to be used for anything.

comment:122 Changed 21 months ago by 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?

comment:123 Changed 21 months ago by daf

  • Cc daf@… added

comment:124 Changed 20 months ago by 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.

comment:125 Changed 20 months ago by free.ekanayaka

  • Cc free@… added

comment:125 Changed 20 months ago by free.ekanayaka

  • Cc free@… added

comment:126 Changed 19 months ago by _plq_

  • Cc burak+twisted@… added

Hi there,

I have 4173-5 (almost) in production use.

I had to change the import line for this to work, is it normal?

https://github.com/plq/spyne/commit/227da556c9b8057738ee6443eacd63aeeb37d240#L1L32

I'm using twisted trunk from github mirror.

Best,

comment:127 Changed 19 months ago by _plq_

Whoops, turns out I had 4173-3 in production.

When I upgraded to 4173-5, here's what I got:

2013-06-09 21:48:50+0300 [HTTPChannel,0,127.0.0.1] Unhandled Error

Traceback (most recent call last):

File "/home/plq/src/github/plq/twisted/twisted/protocols/basic.py", line 581, in dataReceived

why = self.lineReceived(line)

File "/home/plq/src/github/plq/twisted/twisted/web/http.py", line 1611, in lineReceived

self.allContentReceived()

File "/home/plq/src/github/plq/twisted/twisted/web/http.py", line 1686, in allContentReceived

req.requestReceived(command, path, version)

File "/home/plq/src/github/plq/twisted/twisted/web/http.py", line 790, in requestReceived

self.process()

--- <exception caught here> ---

File "/home/plq/src/github/plq/twisted/twisted/web/server.py", line 192, in process

self.render(resrc)

File "/home/plq/src/github/plq/twisted/twisted/web/server.py", line 241, in render

body = resrc.render(self)

File "/home/plq/src/github/plq/spyne/spyne/util/_twisted_ws.py", line 464, in render

protocol.makeConnection(transport)

File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 75, in makeConnection

self.wrappedProtocol.makeConnection(self)

File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 75, in makeConnection

self.wrappedProtocol.makeConnection(self)

File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 73, in makeConnection

Protocol.makeConnection(self, transport)

File "/home/plq/src/github/plq/twisted/twisted/internet/protocol.py", line 460, in makeConnection

self.connectionMade()

File "/home/plq/src/github/plq/spyne/spyne/server/twisted.py", line 347, in _connectionMade

WebSocketsProtocol.connectionMade(self)

File "/home/plq/src/github/plq/spyne/spyne/util/_twisted_ws.py", line 230, in connectionMade

log.msg("Opening connection with %s" % self.transport.getPeer())

File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 94, in getPeer

return self.transport.getPeer()

File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 94, in getPeer

return self.transport.getPeer()

(...)
File "/home/plq/src/github/plq/twisted/twisted/protocols/policies.py", line 94, in getPeer

return self.transport.getPeer()

exceptions.RuntimeError: maximum recursion depth exceeded

comment:129 Changed 18 months ago by therve

  • Branch changed from branches/websocket-4173-3 to branches/websocket-4173-4

(In [39076]) Branching to 'websocket-4173-4'

comment:130 Changed 18 months ago by therve

  • Keywords review added
  • Owner therve deleted

Dave: the headers shouldn't appear, there is an explicit check for that. I don't see it.

plq: 4173-5 is not a branch, so I don't know what you're talking about.

Back for review!

comment:131 Changed 18 months ago by davep

Yep, I think I was using a mix of versions, actually. Looks good!

comment:133 Changed 18 months ago by therve

Ah you should ignore those patches, the work is happening in the SVN repository.

Changed 18 months ago by tom.prince

Example binary amp from browser.

comment:134 in reply to: ↑ 111 ; follow-up: Changed 18 months ago by tom.prince

Replying to 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.)

Well, I managed to create a simple proof-of-conecpt amp-in-the-browser example, using a slightly hacked node-amp and browser-buffer.

comment:135 Changed 17 months ago by tekNico

  • Cc tekNico removed

comment:136 in reply to: ↑ 134 Changed 17 months ago by therve

Replying to tom.prince:

Well, I managed to create a simple proof-of-conecpt amp-in-the-browser example, using a slightly hacked node-amp and browser-buffer.

The branch ought to support binary now, have you tried and encountered issues?

comment:137 Changed 17 months ago by tom.prince

  • Owner set to tom.prince

comment:138 Changed 17 months ago by tom.prince

  • Owner changed from tom.prince to therve

Thanks for your work on this.

  1. Composition is better than inheritence: I think it would make more sense to pass lookupProtocol as an argument, rather than have subclasses override it.
    • A related point: If lookupProtocol is passed/overriden, then the required argument factory is useless.
    • Please allow passing lookupProtocol as an argument.
  2. I don't think using isSecure is the correct way to detect TLSMemoryBIOProtocol.
    • isSecure can be overriden by setHost, and only checks for ISSLTransport. Even if TLSMemoryBIOProtocol is only currently supported (it isn't, see _oldtls) implementation, there could be alternative implemntations. Or, somebody could be instantiating TLSMemoryBIOProtocol directly (rather than via startTLS).
    • I'm not sure how this should be handled. My only though is an isinstance check on transport.protocol. :(
    • I wonder if there should be a sanity check that the wrapped protocol is what we expect, before we replace it.
  3. I haven't looked, but I wonder if this will possibly leak, do to this never finishing the request that initiated the connection. We dissaociate it from the transport, but is there anything else hold a reference to it?
    • Please investigate, and leave a comment explaining why it doesn't, or fix it so that it doesn't.
  4. There is no way to observe PONG frames. Or to send a CLOSE frame with a reason code and message
    • Please add support for thse, or point out how they can be added compatibily at a later date.
  5. Please change the log messages to use format and keyword arguments.
  6. I would be happier if there was more layering. There are lots of instances in twisted currently, where we have a protocol that adds some semantics to the underlying stream, and the provides methods to sennd and receive at that layer (LineReceived is perhaps the most used example of that). I think it would be be better if the object that was the IProtocol wasn't also the object that has frameReceieved called on it.
    • Please split up WebSocketProtocol into a protocol and a transport (which will it self be an IProtocol).
    • Preferably, the transport will only expose the transport interface to the protocol (using proxyForInterface?)
    • Given point 3 above, it would be nice if we could specify that users shuoldn't implement the transport interface themselves (at least not yet), so that we can extend it compatibly (to add stuff like sending pong or close, for example).
  7. Documentation.
    1. lookupProtocol (in whatever form it ends up) should be documented in the narrative documentation.
    2. (optional) It would be nice if the frame-level interface was also documented.
  8. Tests:
    1. test_render doesn't check that that render checks the request headers. (The docstring is inaccurate)
    2. The change to twisted.web.http appears untested.

Please resubmit for review after addressing the above points.

Random thoughts on future extensions:

  • This seems like it could easily be refactored so that the body of render is just a function you can call from render or render_GET of an arbitrary resource. This would allow handling websocket requests by arbitrary resources, i.e. not just leaf nodes, and allow handling of non-websocket requests by that resource as well.
    • This should be handled in a seperate ticket
    • Maybe it would want to be two functions: one to check if it is a websocket (possibly invalid) request, and then one to actually handle the requests.

comment:139 Changed 17 months ago by tom.prince

  • Keywords review removed

comment:140 Changed 17 months ago by therve

  • Keywords review added
  • Owner therve deleted

Alright, thanks for the review. Quick turnaround because I want this to move forward, and I want feedback on the latest refactoring. I split the WebSocketProtocol with a new IWebSocketsReceiver interface to implement, and a provide a WebSocketTransport class. It ought to fix points 4 and 6 in your review.

I've also handled points 1, 2, 5 and 8.1. 8.2 is tested (I believe) by test_renderRealRequest. It leaves point 3 and 7.

comment:141 Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to therve
  1. There are some pydoctor and twistedchecker errors, as well as a error due to missing pyopenssl.
  2. My appologies, I was unclear. When I suggested using format, and keyword arguments for log messages, I meant using the format argument of log.msg, and passing keyword arguments to to log.msg which will get interpolated with % formating.
    log.msg(format="Opening connection with %(peer)s", peer=self.transport.getPeer())
    
  3. SavingEchoReceiver should implement IWebSocketFrameReceiver
  4. IWebSocketFrameReceiver: Is there any reason to specify that WebSocketsTransport wraps a TCP transport? I don't think it is even necessarily true.
  5. Closing a connection: (Sorry for the following being very disjointed, I'm not really sure what this should look like)
    1. WebSocketsTransport.loseConnection takes a reason argument, but doesn't specify its type. It should probably be two arguments, since I seem to recall that the CLOSE frame takes a 2-byte reason code, followed by a UTF-8 encoded reason.
    2. It would be nice if the connection given to connectionLost contained the code and message from the other side.
      • I guess this means there should be a connectionLost.
      • We want it anyway, for a badly behaved peer (or intermediate) closing the connection without sending CLOSE.
    3. If the user doesn't send a CLOSE frame in response to receiving a CLOSE frame, we should send one ourself, before closing the connection.
      • But the RFC specifically allows waiting to reply to a CLOSE, while continuing to send stuff (like an in-progress message).
    4. Does abortConnection want to exposed too?
    5. If the user sends a CLOSE frame themselves, we should probably avoid doing so in loseConnection.
  6. It would be nice to split WebSocketsProtocolWrapper into a socket-facing and protocol-facing parts (or pretend to with something like proxyForInterace.
    • I don't like the use of indiscriminate __getattr__ for proxying stuff like this, but I don't think we have tools for doing it better (see #4700).
    • It almost looks like WebSocketsProtocolWrapper could be made into something that constructs a WebSocketsProtocol, rather than something that subclasses it. Particularly once connectionLost from 13)
    • It occurs to me that we should allow producers to be hooked up to a WebSocketsTransport.
  1. (minor, optional) Should things be called Websocket(Protocol|Resource|...) instead of WebScokets(Protocol|Resource|...)`


Sorry for the incomplete and disjointed review, but I'm out of time for today. :(

I do like how you have split things up here. I think this is a good direction for code in twisted to head.

comment:142 Changed 13 months ago by lvh

As a side note, tom.prince's buffer.js-based approach *does* work, but I was unable to get it working completely for me. It relies on the existence of Uint8Array, which means good browsers. Unfortunately part of the reason I use SockJS in the first place is bad browsers :)

comment:143 Changed 10 months ago by tom.prince

In creating a simple example that connects a websocket to a remote ssh server, I found that I wanted lookupProtocol to be able to return a deferred. Otherwise the protocol returned from it needs to handle data received before it is connected to the other side.

comment:144 follow-up: Changed 8 months ago by MostAwesomeDude

So, uh, who wants to work on this? Am I carrying this down the home stretch?

comment:145 in reply to: ↑ 144 Changed 8 months ago by glyph

Replying to MostAwesomeDude:

So, uh, who wants to work on this? Am I carrying this down the home stretch?

ONLY YOU.

comment:146 follow-ups: Changed 7 months ago by ecnahc515

  • Cc chance.zibolski@… added

I'd like to at least try to finish this up. Firstly, I'm looking at the list of items to resolve in the last review.

#9, #10, #11 look done.

#12) I cannot find a reference to IWebSocketFrameReceiver so I'm not sure what Tom.Prince is referring to for this.

#13) part 1 looks done, but the rest I'm unsure of. Where do we handle the other side sending a CLOSE frame, and where would we handle getting the 13.2 (the code and reason from the other side)?

#14) I'm honestly not sure where exactly to start on that. It doesn't look like that's been started at all.

Any advice is appreciated. I just need a little bit of a nudge in the right direction and I think I can figure out what I need to.

comment:147 in reply to: ↑ 146 Changed 7 months ago by ecnahc515

Replying to ecnahc515:

I'd like to at least try to finish this up. Firstly, I'm looking at the list of items to resolve in the last review.

#9, #10, #11 look done.

#12) I cannot find a reference to IWebSocketFrameReceiver so I'm not sure what Tom.Prince is referring to for this.

#13) part 1 looks done, but the rest I'm unsure of. Where do we handle the other side sending a CLOSE frame, and where would we handle getting the 13.2 (the code and reason from the other side)?

#14) I'm honestly not sure where exactly to start on that. It doesn't look like that's been started at all.

Any advice is appreciated. I just need a little bit of a nudge in the right direction and I think I can figure out what I need to.

After a fresh checkout of the branch I found the #12, and that seems fine too, but that still leaves the rest.

comment:148 in reply to: ↑ 146 Changed 7 months ago by glyph

Replying to ecnahc515:

I'd like to at least try to finish this up. Firstly, I'm looking at the list of items to resolve in the last review.

Thanks very much for your interest. I haven't familiarized myself with this code in quite a while, so I can't personally provide that nudge, but I hope someone else will do so soon :).

Note: See TracTickets for help on using tickets.