Add a web socket client

Add a web socket client

The web socket client is a implemented as a class which is
created from from a HTTP client connection. This way it is
independent from the HTTP client and is just using the HTTP
client.

The protocol handling is mainly re-using the code from the web
socket server.

Change the handling of setting an onNoPendingWrites handler on a
socket output stream. Now setting a new onNoPendingWrites handler
will trigger a callback when there is no pending data to be
written.

Add socket detaching to the HTTP client as well. Add the ability
to get any unparsed data read from the socket for processing
after the protocol upgrade.

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

BUG=dart:2001
TEST=tests/standalone/io/web_socket_test.dart

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

[Anders Johnsen, 2012-05-02 07:51:57]

LGTM

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http.dart#newcode570
runtime/bin/http.dart:570: class DetachedSocket {
The client should never create this object, so I think we should move to
interface + private implementation.

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

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:82: WebSocketClientConnection(HttpClientConnection
conn,
Should we add a convenient constructor that just takes a WebSocket url and maybe
a HttpHeader object?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:105: void set onMessage(void callback(message));
Add 'Object' type to match send.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:121: void set onNoUpgrade(void
callback(HttpClientResponse request));
request->response.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dart
File runtime/bin/websocket_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:655: request.headers.set("Sec-WebSocket-Key",
_generateNonce());
Even though it's not the case, this looks like the nonce is written once and
then forgotten. Maybe change to a call, then read value from field.

[Mads Ager (google), 2012-05-02 08:18:17]

LGTM

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:605: } else if (_contentLength < 0) {
An nothing needs to be done here fore _contentLength == 0?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:773: // TODO(sgjesse): Handle getting the write
handler when using output stream.
Let's file a bug report on this to keep track?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:1237: // TODO(sgjesse): Needs some state checks.
Either add the checks or file a bug to keep track?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
File runtime/bin/socket_stream_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
runtime/bin/socket_stream_impl.dart:115: _setupWaitOnWrite();
I found the name a bit confusing. How about calling this something like
_setupWriteHandler?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
runtime/bin/socket_stream_impl.dart:199: void _setupWaitOnWrite() {
Can we add a few comments? This sets up a write handler that will write all
pending data when the underlying socket is ready for more writes.

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

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:101: * received. The type on [message] is either
[:String:] or
The type on -> The type of

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:130: * Sends a message. The [message] must be a
[:String:] a
[:String:] or a

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dart
File runtime/bin/websocket_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:560: class _WebSocketConnection extends
_WebSocketConnectionBase implements WebSocketConnection {
Long line.

What is the reason to have both _WebSocketConnectionBase and
_WebSocketConnection. There is nothing new here, so can we just rename
_WebSocketConnectionBase to _WebSocketConnection?

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:628: class _WebSocketClientConnection extends
_WebSocketConnectionBase implements WebSocketClientConnection {
Long line.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:655: request.headers.set("Sec-WebSocket-Key",
_generateNonce());
On 2012/05/02 07:51:57, ajohnsen wrote:
> Even though it's not the case, this looks like the nonce is written once and
> then forgotten. Maybe change to a call, then read value from field.

Or have a getter that calculates on first access and caches.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:680: _socketReady(detached.socket,
detached.unparsedData);
Instead of picking out the socket and the unparsedData here just pass the
DetachedSocet to _socketREady? Similarly for the constructors of the connection.

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
File tests/standalone/io/web_socket_test.dart (right):

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
tests/standalone/io/web_socket_test.dart:26: conn.onError = (e) =>
Expect.fail("Unexpected error $e");
Since the error handler throws by default now we could leave this out. But it
might be nice to have it explict?

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
tests/standalone/io/web_socket_test.dart:36: wsconn.onOpen = () {
Could be a one-liner.

[Mads Ager (google), 2012-05-02 08:58:27]

A more high-level comment. We should consider if we can wrap the
WebSocketClientConnection in a WebSocket object that has the client API. I think
what you have in this changelist makes sense for the server and it seems that we
can support the client-side API as a restricted wrapper around the real deal.

[Søren Gjesse, 2012-05-02 10:22:45]

I will try to draft a WebSocket class for presenting the browser API,

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http.dart#newcode570
runtime/bin/http.dart:570: class DetachedSocket {
On 2012/05/02 07:51:57, ajohnsen wrote:
> The client should never create this object, so I think we should move to
> interface + private implementation.

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:605: } else if (_contentLength < 0) {
On 2012/05/02 08:18:17, Mads Ager wrote:
> An nothing needs to be done here fore _contentLength == 0?

Yes - this was a bug. The spec says:

"The presence of a message-body in a request is signaled by the inclusion of a
Content-Length or Transfer-Encoding header field in the request's
message-headers."

So no body is signaled by having neither Content-Length nor Transfer-Encoding.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:773: // TODO(sgjesse): Handle getting the write
handler when using output stream.
On 2012/05/02 08:18:17, Mads Ager wrote:
> Let's file a bug report on this to keep track?

Fixed by setting _socket.outputStream.onNoPendingWrites to null.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/http_impl.dart#new...
runtime/bin/http_impl.dart:1237: // TODO(sgjesse): Needs some state checks.
On 2012/05/02 08:18:17, Mads Ager wrote:
> Either add the checks or file a bug to keep track?

Removed the TODO - not sure which state checks I was thinking about.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
File runtime/bin/socket_stream_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
runtime/bin/socket_stream_impl.dart:115: _setupWaitOnWrite();
On 2012/05/02 08:18:17, Mads Ager wrote:
> I found the name a bit confusing. How about calling this something like
> _setupWriteHandler?

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/socket_stream_impl...
runtime/bin/socket_stream_impl.dart:199: void _setupWaitOnWrite() {
On 2012/05/02 08:18:17, Mads Ager wrote:
> Can we add a few comments? This sets up a write handler that will write all
> pending data when the underlying socket is ready for more writes.

Done.

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

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:82: WebSocketClientConnection(HttpClientConnection
conn,
On 2012/05/02 07:51:57, ajohnsen wrote:
> Should we add a convenient constructor that just takes a WebSocket url and
maybe
> a HttpHeader object?

After having discussed this off-line we will make a WebSocket class which have
the same interface as the WebSocket in the browser and keep the
WebSocketClientConnection as is.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:101: * received. The type on [message] is either
[:String:] or
On 2012/05/02 08:18:17, Mads Ager wrote:
> The type on -> The type of

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:105: void set onMessage(void callback(message));
On 2012/05/02 07:51:57, ajohnsen wrote:
> Add 'Object' type to match send.

Removed Object from send instead.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:121: void set onNoUpgrade(void
callback(HttpClientResponse request));
On 2012/05/02 07:51:57, ajohnsen wrote:
> request->response.

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:130: * Sends a message. The [message] must be a
[:String:] a
On 2012/05/02 08:18:17, Mads Ager wrote:
> [:String:] or a

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dart
File runtime/bin/websocket_impl.dart (right):

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:560: class _WebSocketConnection extends
_WebSocketConnectionBase implements WebSocketConnection {
On 2012/05/02 08:18:17, Mads Ager wrote:
> Long line.
> 
> What is the reason to have both _WebSocketConnectionBase and
> _WebSocketConnection. There is nothing new here, so can we just rename
> _WebSocketConnectionBase to _WebSocketConnection?

The WebSocketConnectionBase is also used as a base class for
WebSocketClientConnection which have additional implementation. Therefore I did
not want this constructor on the base class for WebSocketClientConnection.

Fixed the long line.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:628: class _WebSocketClientConnection extends
_WebSocketConnectionBase implements WebSocketClientConnection {
On 2012/05/02 08:18:17, Mads Ager wrote:
> Long line.

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:655: request.headers.set("Sec-WebSocket-Key",
_generateNonce());
On 2012/05/02 07:51:57, ajohnsen wrote:
> Even though it's not the case, this looks like the nonce is written once and
> then forgotten. Maybe change to a call, then read value from field.

Done.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:655: request.headers.set("Sec-WebSocket-Key",
_generateNonce());
On 2012/05/02 08:18:17, Mads Ager wrote:
> On 2012/05/02 07:51:57, ajohnsen wrote:
> > Even though it's not the case, this looks like the nonce is written once and
> > then forgotten. Maybe change to a call, then read value from field.
> 
> Or have a getter that calculates on first access and caches.

Changed as Anders suggested.

http://codereview.chromium.org/10262031/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:680: _socketReady(detached.socket,
detached.unparsedData);
On 2012/05/02 08:18:17, Mads Ager wrote:
> Instead of picking out the socket and the unparsedData here just pass the
> DetachedSocet to _socketREady? Similarly for the constructors of the
connection.

Done.

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
File tests/standalone/io/web_socket_test.dart (right):

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
tests/standalone/io/web_socket_test.dart:26: conn.onError = (e) =>
Expect.fail("Unexpected error $e");
On 2012/05/02 08:18:17, Mads Ager wrote:
> Since the error handler throws by default now we could leave this out. But it
> might be nice to have it explict?

No, not realy - removed.

http://codereview.chromium.org/10262031/diff/1/tests/standalone/io/web_socket...
tests/standalone/io/web_socket_test.dart:36: wsconn.onOpen = () {
On 2012/05/02 08:18:17, Mads Ager wrote:
> Could be a one-liner.

Done.