code review 156071: Add WebSocket server framework hooked into http.

Add WebSocket server framework hooked into http.

[ukai, 2009-11-19 03:35:26]

Hello r, rsc,

I'd like you to review the following change.

[rsc, 2009-11-20 01:06:43]

Thanks for writing this.

http://codereview.appspot.com/156071/diff/1005/6
File src/pkg/Make.deps (right):

http://codereview.appspot.com/156071/diff/1005/6#newcode8
src/pkg/Make.deps:8: compress/gzip.install: bufio.install compress/flate.install
hash.install hash/crc32.install io.install os.install
Please do

cd $GOROOT
hg file -d 156071 src/pkg/Make.deps

We changed Make.deps to be automatically generated.

http://codereview.appspot.com/156071/diff/1005/7
File src/pkg/Makefile (right):

http://codereview.appspot.com/156071/diff/1005/7#newcode114
src/pkg/Makefile:114: websocket\
It would be good to write a test.
See the rpc package for an example
of writing a network test using an http server.

http://codereview.appspot.com/156071/diff/1005/9
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1005/9#newcode9
src/pkg/websocket/websocket.go:9: //   The Web Wocket protocol:
http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol
s/Wocket/Socket/

though I do wish it were called Web Wocket.

http://codereview.appspot.com/156071/diff/1005/9#newcode27
src/pkg/websocket/websocket.go:27: 
This is an interesting protocol.  I think that instead of using
channels it would make more sense to present the connection
as a simple read/write interface.

type Handler interface {
    ServeWebSocket(c *Conn);
}

type Conn struct {
    // metadata about a connection
}

func (c *Conn) Read(b []byte) (n int, err os.Error) {
}

func (c *Conn) Write ... {
}

etc.

var _ net.Conn = (*Conn)(nil);  // compile-time check that *Conn satisifies
net.Conn.

http://codereview.appspot.com/156071/diff/1005/9#newcode217
src/pkg/websocket/websocket.go:217: type Dispatcher struct {
I don't think you need the complexity of the backup handler.

I think you could toss the Handler above and use

type Handler func(*Conn)

func (f Handler) ServeHTTP(c *http.Conn, req *http.Request) {
    if req.Method != "GET" || req.Proto != "HTTP/1.1" || ... {
        c.WriteHeader(StatusNotFound);
        io.WriteString(c, "must use websocket to connect here");
        return;
    }

    wc := &Conn{
        ... fill in websocket Conn
    }
    f(wc);
}

Then people who want to register handlers would just
handle on a different URL from the normal content:

    http.Handle("/myWebSocket", websocket.Handler(f));

[ukai, 2009-11-20 10:15:56]

PTAL

http://codereview.appspot.com/156071/diff/1005/6
File src/pkg/Make.deps (right):

http://codereview.appspot.com/156071/diff/1005/6#newcode8
src/pkg/Make.deps:8: compress/gzip.install: bufio.install compress/flate.install
hash.install hash/crc32.install io.install os.install
On 2009/11/20 01:06:43, rsc wrote:
> Please do
> 
> cd $GOROOT
> hg file -d 156071 src/pkg/Make.deps
> 
> We changed Make.deps to be automatically generated.
> 

Done.

http://codereview.appspot.com/156071/diff/1005/7
File src/pkg/Makefile (right):

http://codereview.appspot.com/156071/diff/1005/7#newcode114
src/pkg/Makefile:114: websocket\
On 2009/11/20 01:06:43, rsc wrote:
> It would be good to write a test.
> See the rpc package for an example
> of writing a network test using an http server.
> 

Done.

http://codereview.appspot.com/156071/diff/1005/9
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1005/9#newcode9
src/pkg/websocket/websocket.go:9: //   The Web Wocket protocol:
http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol
On 2009/11/20 01:06:43, rsc wrote:
> s/Wocket/Socket/
> 
> though I do wish it were called Web Wocket.
> 

Done.

http://codereview.appspot.com/156071/diff/1005/9#newcode27
src/pkg/websocket/websocket.go:27: 
On 2009/11/20 01:06:43, rsc wrote:
> This is an interesting protocol.  I think that instead of using
> channels it would make more sense to present the connection
> as a simple read/write interface.
> 
> type Handler interface {
>     ServeWebSocket(c *Conn);
> }
> 
> type Conn struct {
>     // metadata about a connection
> }
> 
> func (c *Conn) Read(b []byte) (n int, err os.Error) {
> }
> 
> func (c *Conn) Write ... {
> }
> 
> etc.
> 
> var _ net.Conn = (*Conn)(nil);  // compile-time check that *Conn satisifies
> net.Conn.
> 
> 

I don't think it has the same semantics with net.Conn.
WebSocket provides message in frame rather than arbitrary byte array read/write
like plain socket.

http://codereview.appspot.com/156071/diff/1005/9#newcode217
src/pkg/websocket/websocket.go:217: type Dispatcher struct {
On 2009/11/20 01:06:43, rsc wrote:
> I don't think you need the complexity of the backup handler.

Hmm, but AFAIK other websocket implementations (pywebsocket and gse) supports
this feature.
> 
> I think you could toss the Handler above and use
> 
> type Handler func(*Conn)
> 
> func (f Handler) ServeHTTP(c *http.Conn, req *http.Request) {
>     if req.Method != "GET" || req.Proto != "HTTP/1.1" || ... {
>         c.WriteHeader(StatusNotFound);
>         io.WriteString(c, "must use websocket to connect here");
>         return;
>     }
> 
>     wc := &Conn{
>         ... fill in websocket Conn
>     }
>     f(wc);
> }
> 
> Then people who want to register handlers would just
> handle on a different URL from the normal content:
> 
>     http.Handle("/myWebSocket", websocket.Handler(f));
> 
>

[rsc, 2009-11-20 10:22:34]

There's nothing that says each Read or Write can't
be framed on the way out.  If you have a net.Conn
corresponding to a UDP connection, each Write gets
put in a different UDP packet.

You can define that each Read returns one frame
(assuming the read is big enough) and that each Write
goes out as one frame.

I can believe that other frameworks allow the backup
http handler, but I don't see why it's necessary
(the person writing the server can just use a different
path for websocket vs actual content), and I'd rather
leave it out to keep things simple, until an actual need
arises.

[ukai, 2009-11-21 04:04:16]

PTAL

On 2009/11/20 10:22:34, rsc wrote:
> There's nothing that says each Read or Write can't
> be framed on the way out.  If you have a net.Conn
> corresponding to a UDP connection, each Write gets
> put in a different UDP packet.
> 
> You can define that each Read returns one frame
> (assuming the read is big enough) and that each Write
> goes out as one frame.

Ok, I see.

> 
> I can believe that other frameworks allow the backup
> http handler, but I don't see why it's necessary
> (the person writing the server can just use a different
> path for websocket vs actual content), and I'd rather
> leave it out to keep things simple, until an actual need
> arises.

Ok.

[rsc, 2009-11-24 00:11:12]

http://codereview.appspot.com/156071/diff/1020/2005
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1020/2005#newcode66
src/pkg/websocket/websocket.go:66: func (ws *Conn) Read(msg []byte) (n int, err
os.Error) {
Instead of using the channels here, this should just be
the body of read.  Similarly write should just be function calls.
There should be no need for channels.

http://codereview.appspot.com/156071/diff/1020/2005#newcode193
src/pkg/websocket/websocket.go:193: var _ net.Conn = (*Conn)(nil)	//
compile-time check that *Conn statisfies net.Conn.
s/statisfies/implements/

[ukai, 2009-11-24 01:12:59]

Thanks for review.
PTAL.

http://codereview.appspot.com/156071/diff/1020/2005
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1020/2005#newcode66
src/pkg/websocket/websocket.go:66: func (ws *Conn) Read(msg []byte) (n int, err
os.Error) {
On 2009/11/24 00:11:12, rsc wrote:
> Instead of using the channels here, this should just be
> the body of read.  Similarly write should just be function calls.
> There should be no need for channels.
> 

I thought it would be necessary to make read/write msg atomic on the underlying
rwc, if client uses websocket.Conn on different threads.  Is it client
responsibility?

http://codereview.appspot.com/156071/diff/1020/2005#newcode193
src/pkg/websocket/websocket.go:193: var _ net.Conn = (*Conn)(nil)	//
compile-time check that *Conn statisfies net.Conn.
On 2009/11/24 00:11:12, rsc wrote:
> s/statisfies/implements/
> 

Done.

[rsc, 2009-11-25 00:06:49]

On 2009/11/24 01:12:59, ukai wrote:
> Thanks for review.
> PTAL.
> 
> http://codereview.appspot.com/156071/diff/1020/2005
> File src/pkg/websocket/websocket.go (right):
> 
> http://codereview.appspot.com/156071/diff/1020/2005#newcode66
> src/pkg/websocket/websocket.go:66: func (ws *Conn) Read(msg []byte) (n int,
err
> os.Error) {
> On 2009/11/24 00:11:12, rsc wrote:
> > Instead of using the channels here, this should just be
> > the body of read.  Similarly write should just be function calls.
> > There should be no need for channels.
> > 
> 
> I thought it would be necessary to make read/write msg atomic on the
underlying
> rwc, if client uses websocket.Conn on different threads.  Is it client
> responsibility?

It's okay for this to be the client's responsibility.
If this turns out to be a problem then it might
make sense to put in a lock.

Russ

[ukai, 2009-11-25 00:32:35]

On Wed, Nov 25, 2009 at 9:06 AM, <rsc@golang.org> wrote:

> On 2009/11/24 01:12:59, ukai wrote:
>
>> Thanks for review.
>> PTAL.
>>
>
>  http://codereview.appspot.com/156071/diff/1020/2005
>> File src/pkg/websocket/websocket.go (right):
>>
>
>  http://codereview.appspot.com/156071/diff/1020/2005#newcode66
>> src/pkg/websocket/websocket.go:66: func (ws *Conn) Read(msg []byte) (n
>>
> int, err
>
>> os.Error) {
>> On 2009/11/24 00:11:12, rsc wrote:
>> > Instead of using the channels here, this should just be
>> > the body of read.  Similarly write should just be function calls.
>> > There should be no need for channels.
>> >
>>
>
>  I thought it would be necessary to make read/write msg atomic on the
>>
> underlying
>
>> rwc, if client uses websocket.Conn on different threads.  Is it client
>> responsibility?
>>
>
> It's okay for this to be the client's responsibility.
> If this turns out to be a problem then it might
> make sense to put in a lock.
>

I see.
So, does it look good?

-- 
ukai

[rsc, 2009-11-25 01:25:19]

Looks pretty good.

http://codereview.appspot.com/156071/diff/1026/3004
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/156071/diff/1026/3004#newcode32
src/pkg/websocket/client.go:32: func NewClient(resourceName, host, origin,
location, protocol string, rwc io.ReadWriteCloser) (ws *Conn, err os.Error) {
This function has too many arguments to expect clients
to use it.  Maybe it should be lowercase for now
until we know what clients will want from websocket.NewClient?

I.e. rename to newClient.

http://codereview.appspot.com/156071/diff/1026/3005
File src/pkg/websocket/server.go (right):

http://codereview.appspot.com/156071/diff/1026/3005#newcode25
src/pkg/websocket/server.go:25: //  func EchoBackServer(ws *websocket.Conn) {
s/EchoBack/Echo/

http://codereview.appspot.com/156071/diff/1026/3005#newcode26
src/pkg/websocket/server.go:26: //       for {
Can replace entire for loop with

io.Copy(ws, ws);

http://codereview.appspot.com/156071/diff/1026/3005#newcode39
src/pkg/websocket/server.go:39: //    http.Handle("/hello",
websocket.Handler(EchoBackServer));
s/EchoBack/Echo/
s/hello/echo/

http://codereview.appspot.com/156071/diff/1026/3006
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1026/3006#newcode42
src/pkg/websocket/websocket.go:42: func New(origin, location, protocol string,
buf *bufio.ReadWriter, rwc io.ReadWriteCloser) *Conn {
I think this should probably be hidden for now
until there is a clear need for a client to use it.

newConn

http://codereview.appspot.com/156071/diff/1026/3006#newcode105
src/pkg/websocket/websocket.go:105: buf := make([]byte, len(msg)+2);
This framing is wrong if the message has a 0xFF byte in it.
It seems like for a general Write method the thing to do is
use the length-prefixed form, which is more efficient for
the reader too.  Also instead of doing bytes.Copy you
can write directly to the ws.buf.

ws.buf.WriteByte() multiple times to send the length
ws.buf.Write(msg)
ws.buf.Flush()

[ukai, 2009-11-25 01:43:13]

Thanks for review!

http://codereview.appspot.com/156071/diff/1026/3004
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/156071/diff/1026/3004#newcode32
src/pkg/websocket/client.go:32: func NewClient(resourceName, host, origin,
location, protocol string, rwc io.ReadWriteCloser) (ws *Conn, err os.Error) {
On 2009/11/25 01:25:19, rsc wrote:
> This function has too many arguments to expect clients
> to use it.  Maybe it should be lowercase for now
> until we know what clients will want from websocket.NewClient?
> 
> I.e. rename to newClient.
> 

Done.

http://codereview.appspot.com/156071/diff/1026/3005
File src/pkg/websocket/server.go (right):

http://codereview.appspot.com/156071/diff/1026/3005#newcode25
src/pkg/websocket/server.go:25: //  func EchoBackServer(ws *websocket.Conn) {
On 2009/11/25 01:25:19, rsc wrote:
> s/EchoBack/Echo/
> 

Done.

http://codereview.appspot.com/156071/diff/1026/3005#newcode26
src/pkg/websocket/server.go:26: //       for {
On 2009/11/25 01:25:19, rsc wrote:
> Can replace entire for loop with
> 
> io.Copy(ws, ws);
> 

Done.

http://codereview.appspot.com/156071/diff/1026/3005#newcode39
src/pkg/websocket/server.go:39: //    http.Handle("/hello",
websocket.Handler(EchoBackServer));
On 2009/11/25 01:25:19, rsc wrote:
> s/EchoBack/Echo/
> s/hello/echo/
> 

Done.

http://codereview.appspot.com/156071/diff/1026/3006
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1026/3006#newcode42
src/pkg/websocket/websocket.go:42: func New(origin, location, protocol string,
buf *bufio.ReadWriter, rwc io.ReadWriteCloser) *Conn {
On 2009/11/25 01:25:19, rsc wrote:
> I think this should probably be hidden for now
> until there is a clear need for a client to use it.
> 
> newConn
> 

Done.

http://codereview.appspot.com/156071/diff/1026/3006#newcode105
src/pkg/websocket/websocket.go:105: buf := make([]byte, len(msg)+2);
On 2009/11/25 01:25:19, rsc wrote:
> This framing is wrong if the message has a 0xFF byte in it.

WebSocket only supports UTF-8 as message data, so 0xFF should not be appeared in
the msg.

> It seems like for a general Write method the thing to do is
> use the length-prefixed form, which is more efficient for
> the reader too.  Also instead of doing bytes.Copy you
> can write directly to the ws.buf.

length-prefixed form is defined in the spec, but it is defined for future use.
Current spec says it should be discarded the length-prefixed frame, so we can't
use it.

> 
> ws.buf.WriteByte() multiple times to send the length
> ws.buf.Write(msg)
> ws.buf.Flush()
> 
>

[rsc, 2009-11-25 02:02:52]

LGTM

Please make the one fix below (check for error in Write)
and I will apply it.

http://codereview.appspot.com/156071/diff/1033/1038
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1033/1038#newcode107
src/pkg/websocket/websocket.go:107: ws.buf.Flush();
err = ws.buf.Flush();
return len(msg), err;

[ukai, 2009-11-25 02:10:59]

Thanks for review!

http://codereview.appspot.com/156071/diff/1033/1038
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/156071/diff/1033/1038#newcode107
src/pkg/websocket/websocket.go:107: ws.buf.Flush();
On 2009/11/25 02:02:52, rsc wrote:
> err = ws.buf.Flush();
> return len(msg), err;
> 

Done.

[rsc, 2009-11-29 22:22:48]

*** Submitted as http://code.google.com/p/go/source/detail?r=657a1a72a243 ***

Add WebSocket server framework hooked into http.

R=r, rsc
http://codereview.appspot.com/156071

Committer: Russ Cox <rsc@golang.org>

[sergio (larces), 2009-11-30 01:26:44]

Just a note on the Makefile (for you reviewers remember to check it, not to
resurrect issue #115 every time new code is added)

Right now, besides this, there are two other recently added Makefiles with the
same issue:
pkg/exp/draw/x11/Makefile
pkg/crypto/md4/Makefile

I created a CL at http://codereview.appspot.com/162055 to address these two
files, but I am leaving this one for ukai to fix :)

thanks,
sergio.

http://codereview.appspot.com/156071/diff/3032/1054
File src/pkg/websocket/Makefile (right):

http://codereview.appspot.com/156071/diff/3032/1054#newcode1
src/pkg/websocket/Makefile:1: include $(GOROOT)/src/Make.$(GOARCH)
here we should have include ../../Make.$(GOARCH) to prevent it from breaking if
$(GOROOT) contains whitespaces

http://codereview.appspot.com/156071/diff/3032/1054#newcode9
src/pkg/websocket/Makefile:9: include $(GOROOT)/src/Make.pkg
same thing here. include ../../Make.pkg