Initial web socket server implementation

Initial web socket server implementation

This implements the web socket protocol as a request handler for
the HTTP server. The HTTP server have been updated with the
ability of getting the socket when a HTTP upgrade request is
processed. This way the web socket implementation is not builtin
to the HTTP server.

The API on the server is:

interface WebSocketConnection {
  void set onMessage(void callback(Object message));
  void set onClosed(void callback(int status, String reason));
  void set onError(void callback(e));

  send(Object message);
  close([int status, String reason]);
}

This API currently does not have any streaming but collects the
entire message before invoking the onMessage callback. The
current browser web socket API
(http://dev.w3.org/html5/websockets/) works the same way. The
underlying web socket protocol can have unbounded messages so
long term this probably have to change. But for now the only
likely client for a web socket server is a browser.

Currently this is missing tests. The implementation has been
tested by running the WebSocketTest.dart script and connecting
using a browser. This testing have been done using Chrome 19 and
Firefox 11.

I will be adding tests in separate change lists.

The SHA-1 and base 64 code (initially written by
ajohnsen@google.com) can be removed when these algorithms is
available in the dart:crypto.

R=ager@google.com, ajohnsen@google.com, vsm@google.com, jacobr@google.com

BUG=dart:2001
TEST=none

Committed: https://code.google.com/p/dart/source/detail?r=6998

[Anders Johnsen, 2012-04-24 12:20:47]

First drive-by comments.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:393: Socket protocolUpgrade();
Further comment that this can be done after the HTTP Upgrade header have been
seen.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:416: //WebSocketConnection openWebSocket(protocols, String
host, int port, String path);
Did you mean to include this?

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
File tests/standalone/src/io/WebSocketTest.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
tests/standalone/src/io/WebSocketTest.dart:11: server.listen("0.0.0.0", 1234);
Could you use '0' for port, so we don't conflict with other tests / programs?

[Mads Ager (google), 2012-04-25 10:41:54]

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/base64.dart
File runtime/bin/base64.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/base64.dar...
runtime/bin/base64.dart:17: // Endianness matters?
Did you resolve this? If not it should probably be a TODO.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:393: Socket protocolUpgrade();
On 2012/04/24 12:20:47, ajohnsen wrote:
> Further comment that this can be done after the HTTP Upgrade header have been
> seen.

The name makes it sound like this is requesting a protocol upgrade. Can we
rename to something like upgradedSocket? Or maybe just disconnectSocket?

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
File runtime/bin/websocket_impl.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:113: // TODO(sgjesse): Handle ping.
How about throwing an UnsupportedOperation exception or something like that?

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:117: // TODO(sgjesse): Handle pong.
Ditto.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:133: _ len = 0;
_ len -> _len !

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:136: _ len = 0;
Ditto!

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:158: // The payload is not handled one byte at
the time but in blocks.
at the time -> at a time

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:250: if (_state == START || _state == CLOSED ||
_state == FAILURE) return;
Should this change the state of the socket somehow? Right now it is just ignored
basically.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:270: // Check whether there is any payload. If
not indicate empty message or
Indentation and the comment should be completed.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:421: // Close the socket when the close framehas
been sent - if it
framehas -> frame has

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
File tests/standalone/src/io/WebSocketTest.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
tests/standalone/src/io/WebSocketTest.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
Since this test is not finished yet, maybe you should leave it out of the commit
and only put it in when it is ready to be run?

[Søren Gjesse, 2012-04-25 13:57:14]

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/base64.dart
File runtime/bin/base64.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/base64.dar...
runtime/bin/base64.dart:17: // Endianness matters?
On 2012/04/25 10:41:55, Mads Ager wrote:
> Did you resolve this? If not it should probably be a TODO.

Old bogus comment - removed. The input is bytes, so there are no endianness
issues.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:393: Socket protocolUpgrade();
On 2012/04/24 12:20:47, ajohnsen wrote:
> Further comment that this can be done after the HTTP Upgrade header have been
> seen.

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:393: Socket protocolUpgrade();
On 2012/04/25 10:41:55, Mads Ager wrote:
> On 2012/04/24 12:20:47, ajohnsen wrote:
> > Further comment that this can be done after the HTTP Upgrade header have
been
> > seen.
> 
> The name makes it sound like this is requesting a protocol upgrade. Can we
> rename to something like upgradedSocket? Or maybe just disconnectSocket?

Changed name to detachSocket.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:416: //WebSocketConnection openWebSocket(protocols, String
host, int port, String path);
On 2012/04/24 12:20:47, ajohnsen wrote:
> Did you mean to include this?

No, removed.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
File runtime/bin/websocket_impl.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:113: // TODO(sgjesse): Handle ping.
On 2012/04/25 10:41:55, Mads Ager wrote:
> How about throwing an UnsupportedOperation exception or something like that?

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:117: // TODO(sgjesse): Handle pong.
On 2012/04/25 10:41:55, Mads Ager wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:133: _ len = 0;
On 2012/04/25 10:41:55, Mads Ager wrote:
> _ len -> _len !

Year - I discovered this when writing the tests.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:136: _ len = 0;
On 2012/04/25 10:41:55, Mads Ager wrote:
> Ditto!

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:158: // The payload is not handled one byte at
the time but in blocks.
On 2012/04/25 10:41:55, Mads Ager wrote:
> at the time -> at a time

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:250: if (_state == START || _state == CLOSED ||
_state == FAILURE) return;
On 2012/04/25 10:41:55, Mads Ager wrote:
> Should this change the state of the socket somehow? Right now it is just
ignored
> basically.

Set the state to CLOSED.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:270: // Check whether there is any payload. If
not indicate empty message or
On 2012/04/25 10:41:55, Mads Ager wrote:
> Indentation and the comment should be completed.

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:421: // Close the socket when the close framehas
been sent - if it
On 2012/04/25 10:41:55, Mads Ager wrote:
> framehas -> frame has

Done.

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
File tests/standalone/src/io/WebSocketTest.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
tests/standalone/src/io/WebSocketTest.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2012/04/25 10:41:55, Mads Ager wrote:
> Since this test is not finished yet, maybe you should leave it out of the
commit
> and only put it in when it is ready to be run?

I will do that.

https://chromiumcodereview.appspot.com/10205012/diff/1/tests/standalone/src/i...
tests/standalone/src/io/WebSocketTest.dart:11: server.listen("0.0.0.0", 1234);
On 2012/04/24 12:20:47, ajohnsen wrote:
> Could you use '0' for port, so we don't conflict with other tests / programs?

Sure - I had the fixed port for testing with Chrome and Firefox.

[Mads Ager (google), 2012-04-25 14:26:05]

lgtm

[vsm, 2012-04-25 16:45:18]

https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
runtime/bin/websocket.dart:46: void set onMessage(void callback(Object
message));
Consider making this consistent with onError below and omitting "Object".

FWIW, we've been consistently using a "Dynamic" type / no type in the
dom/html/ui APIs in this sort of case instead of Object.  The rationale is the
Object should indicate that any type is valid while Dynamic indicates that the
precise type cannot be specified.

https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
runtime/bin/websocket.dart:52: void set onClosed(void callback(int status,
String reason));
onClose to be consistent with w3c?

https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
runtime/bin/websocket.dart:60: /**
Does onOpen (from the spec) make sense here?

[Sean Eagan, 2012-04-25 18:14:35]

Great stuff!  I had a couple ideas to improve WebSocketConnection if you don't
mind me commenting here:

It seems like a given web socket connection would generally only communicate
using text (String) or binary (List<int>) messages, as opposed to both.  The
current API might lead to a lot of boiler plate in the onMessage handlers:

void messageHandler(Object message) {
  if(message === null) //...
  else if(message is List<int>) //...
  else if(message is String) //...
}

It might be better to have separate handlers for each message type, and to
convert nulls to empty string or empty List, to avoid NPEs and null checks, by
replacing onMessage with:

void set onText(void callback(String message));
void set onBinary(void callback(List<int> message));

and throw exceptions if a given message type is received and no handler is set.

Similarly, it might be better to have separate send methods for each type, this
also allows one to specify whether an empty (null) message should be sent as
binary or text.  Also could make the argument optional to ease passing empty
messages:

void sendText([String message]);
void sendBinary([List<int> message]);

On 2012/04/25 16:45:18, vsm wrote:
>
https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
> File runtime/bin/websocket.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
> runtime/bin/websocket.dart:46: void set onMessage(void callback(Object
> message));
> Consider making this consistent with onError below and omitting "Object".
> 
> FWIW, we've been consistently using a "Dynamic" type / no type in the
> dom/html/ui APIs in this sort of case instead of Object.  The rationale is the
> Object should indicate that any type is valid while Dynamic indicates that the
> precise type cannot be specified.
> 
>
https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
> runtime/bin/websocket.dart:52: void set onClosed(void callback(int status,
> String reason));
> onClose to be consistent with w3c?
> 
>
https://chromiumcodereview.appspot.com/10205012/diff/8001/runtime/bin/websock...
> runtime/bin/websocket.dart:60: /**
> Does onOpen (from the spec) make sense here?

[Søren Gjesse, 2012-04-26 09:37:44]

Thanks for the review

seaneagan1:
Thanks for the comments. For this API we are currently trying to have some
consistency with the w3c client API from http://dev.w3.org/html5/websockets/.
However as you state the most common case will be to handle only text or only
binary messages on a given handler. I would like to address this through adding
a property on the WebSocketHandler. You can set whether to expect test or binary
messages, and if an unexpected type is received the WebSocketHandler will just
close the web socket with an error. You can then rely on the message passed to
onMessage will always be of the expected type. I will address that as a separate
change and I hope it will work for you.

Also I will get rid of the null messages is a separate change, as thet do not
make much sense. A message is marked as text or binary so an empty message must
be an empty string or an empty integer list.

http://codereview.chromium.org/10205012/diff/8001/runtime/bin/websocket.dart
File runtime/bin/websocket.dart (right):

http://codereview.chromium.org/10205012/diff/8001/runtime/bin/websocket.dart#...
runtime/bin/websocket.dart:46: void set onMessage(void callback(Object
message));
On 2012/04/25 16:45:18, vsm wrote:
> Consider making this consistent with onError below and omitting "Object".
> 
> FWIW, we've been consistently using a "Dynamic" type / no type in the
> dom/html/ui APIs in this sort of case instead of Object.  The rationale is the
> Object should indicate that any type is valid while Dynamic indicates that the
> precise type cannot be specified.

Done. I agree with the distinction between Object and Dynamic - we have tried to
do the same in dart:io.

http://codereview.chromium.org/10205012/diff/8001/runtime/bin/websocket.dart#...
runtime/bin/websocket.dart:52: void set onClosed(void callback(int status,
String reason));
On 2012/04/25 16:45:18, vsm wrote:
> onClose to be consistent with w3c?

This is problematic. All over dart:io we are using onClosed (with the d), and
for that reason I will leave it with the d for now.

http://codereview.chromium.org/10205012/diff/8001/runtime/bin/websocket.dart#...
runtime/bin/websocket.dart:60: /**
On 2012/04/25 16:45:18, vsm wrote:
> Does onOpen (from the spec) make sense here?

As this is the server side the onOpen corresponds to the onConnection on the
WebSocketHandler interface above. On the server you get the connection when it
is already established, so it cannot be part of the WebSocketConnection object.
I have renamed onConnection to onOpen on the WebSocketHandler interface to be
somewhat consistent with w3c.

[Sean Eagan, 2012-04-26 13:14:09]

On 2012/04/26 09:37:44, Søren Gjesse wrote:
> seaneagan1:
> Thanks for the comments. For this API we are currently trying to have some
> consistency with the w3c client API from http://dev.w3.org/html5/websockets/.

I think just developing the best API possible, and then, if necessary, changing
the client API in dart:html to be consistent (similar to
http://dartbug.com/2677) would be welcome.

> However as you state the most common case will be to handle only text or only
> binary messages on a given handler. I would like to address this through
adding
> a property on the WebSocketHandler. You can set whether to expect test or
binary
> messages, and if an unexpected type is received the WebSocketHandler will just
> close the web socket with an error. You can then rely on the message passed to
> onMessage will always be of the expected type. I will address that as a
separate
> change and I hope it will work for you.

Sounds good, a small tweak, rather than a property to designate text vs. binary,
how about a type parameter:

interface WebSocketHandler<T> {
  //...
  void set onOpen(callback(WebSocketConnection<T> connection));
}

interface WebSocketConnection<T> {
  void set onMessage(void callback(T message));
  send T message);
}

and then add sub-interfaces whose implementation constrain the messages to only
text (String) or only binary (List<int>):

interface WebSocketTextHandler extends WebSocketHandler<String> //...
interface WebSocketBinaryHandler extends WebSocketHandler<List<int>> //...