code review 2302042: Fix websocket's Read to handle reading a fragment of a ...

Fix websocket's Read to handle reading a fragment of a frame.

It remembers that there is more data coming and returns that data
to the next Read.
Fixes issue 1145.

[ukai, 2010-09-30 09:41:40]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-09-30 17:57:12]

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket.go
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket.g...
src/pkg/websocket/websocket.go:75: if ws.frameText != nil {
I'm a little confused about what's going on here.
I think there's more state machine than necessary,
and there are still E2BIG errors.  I don't believe 
there should ever be an E2BIG.  Also note that
copy(x, y) copies the smaller of len(x), len(y) bytes
and returns the number of bytes copied.  Some of
the calls to copy in this code are missing checks of
how many bytes got copied.

Each Read needs to:
   * start a new read if needed (figure out frame, etc)
   * execute some fraction of the read

I think something like the below will be simpler if fleshed out:

func (ws *Conn) Read(msg []byte) (n int, err os.Error) {
	// beginning of frame, possibly
	if len(ws.data) == 0 && ws.frame == 0 && !ws.text {
		b = next byte
		if b&0x80 == 0 {
			ws.text = true
		} else {
			ws.frame = ...
		}
	}

	switch {
	case ws.text && len(ws.data) == 0:
		// in text mode; read next chunk and leave as ws.data
		line, err := ws.buf.ReadSlice(0xff)
		if err != nil {
			if len(line) == 0 {
				return 0, err
			}
			ws.data = line
		} else {
			ws.data = line[:len(line)-1]  // drop 0xff
			ws.text = false
		}
		fallthrough
	
	case len(ws.data) > 0:
		// satisfy read from ws.data
		... copy ws.data into msg, updating n, ws.data, ...
		return n, nil
	}

	// in counted length frame; read directly into msg
	... read smaller of len(msg), ws.frame bytes into msg, return ...
}

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket.g...
src/pkg/websocket/websocket.go:86: if (ws.frameByte & 0x80) == 0x80 {
() are unnecessary

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket.g...
src/pkg/websocket/websocket.go:99: if (c & 0x80) == 0 {
() are unnecessary

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket_t...
File src/pkg/websocket/websocket_test.go (right):

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket_t...
src/pkg/websocket/websocket_test.go:224: t.Errorf("Read: expected os.E2BIG got
%v", err)
Why should Read ever return E2BIG?

[ukai, 2010-10-01 06:37:29]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[ukai, 2010-10-01 06:39:19]

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket_t...
File src/pkg/websocket/websocket_test.go (right):

http://codereview.appspot.com/2302042/diff/2001/src/pkg/websocket/websocket_t...
src/pkg/websocket/websocket_test.go:224: t.Errorf("Read: expected os.E2BIG got
%v", err)
On 2010/09/30 17:57:12, rsc1 wrote:
> Why should Read ever return E2BIG?

websocket is not stream-based, but message-based, so user would want to know
message boundary. what would be the good way to indicate Read() returns whole
message or client needs more Read() ?

[rsc1, 2010-10-01 15:58:41]

Please add a test of the length skipping code.

[rsc1, 2010-10-01 16:04:02]

http://codereview.appspot.com/2302042/diff/9001/src/pkg/websocket/websocket.go
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/2302042/diff/9001/src/pkg/websocket/websocket.g...
src/pkg/websocket/websocket.go:102: ws.data = line
Websocket is such a bizarre protocol.
I forgot that all this stuff is supposed to be skipped.

If ReadSlice fills the buffer without finding an 0xff,
it returns what it has and we end up here.  We need to
remember in this case that we're still looking for
the final 0xff so that we don' try to begin a new frame
on the next call or loop iteration (that's what the ws.text
flag was for in the sketch I sent).

I'd suggest tweaking the big Frame loop here to say

for !ws.reading && len(ws.data) == 0 {
    ...
    // In text mode
    if b != 0 {
        loop on ReadByte until 0xff, discarding
        continue
    }
    ws.reading = true
    break
}

if len(ws.data) == 0 {
    ReadSlice, maybe clear ws.reading if we get 0xff
}

n = copy ...

[rsc, 2010-10-01 16:15:02]

>> Why should Read ever return E2BIG?
>
> websocket is not stream-based, but message-based, so user would want to
> know message boundary. what would be the good way to indicate Read()
> returns whole message or client needs more Read() ?

Typically this is done by making Read stop
at message boundaries and relying on the user
to read big enough pieces.  If the user passes
a buffer that is too small, then the message
just comes back in two different Reads.

Russ

[ukai, 2010-10-04 05:24:30]

On 2010/10/01 16:15:02, rsc wrote:
> >> Why should Read ever return E2BIG?
> >
> > websocket is not stream-based, but message-based, so user would want to
> > know message boundary. what would be the good way to indicate Read()
> > returns whole message or client needs more Read() ?
> 
> Typically this is done by making Read stop
> at message boundaries and relying on the user
> to read big enough pieces.  If the user passes
> a buffer that is too small, then the message
> just comes back in two different Reads.

So, in TestSamllBuffer, the second_msg should be "hello, world", instead of
"world" ?  how to distinguish if the stream is "\0hello, \xff\0hello, world\xff"
?
how would the user know that he passes a small buffer for the first Read()?

[rsc, 2010-10-04 10:30:24]

> So, in TestSamllBuffer, the second_msg should be "hello, world", instead
> of "world" ?  how to distinguish if the stream is "\0hello, \xff\0hello,
> world\xff" ?
> how would the user know that he passes a small buffer for the first
> Read()?

He won't, but he also won't lose data if reading
a large message a chunk at a time, as in the original
bug report.  The implementation of the Reader interface
should not deviate from the standard Read.  The fact
that it was doing so is why JSON broke.

If you'd like to add a ReadMessage method with
the same signature that returns E2BIG if there is
not room for an entire message, that's fine, but it's
not what Read is.

Russ

[ukai, 2010-10-04 10:53:59]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[ukai, 2010-10-18 07:44:50]

ping.

I think Read() for small buffer returns data for the buffer, and next Read()
returns rest of data in the frame (and I think JSON's code expects it), and this
CL changed to do it.
Do I need any more fixes?

thanks.

[rsc1, 2010-10-18 15:13:43]

LGTM

Please fix the one thing below.
(If err == nil then it succeeded and there is definitely a 0xff.
If err != nil then ws.data might have len 0, making the array
access crash.)

http://codereview.appspot.com/2302042/diff/22001/src/pkg/websocket/websocket.go
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/2302042/diff/22001/src/pkg/websocket/websocket....
src/pkg/websocket/websocket.go:114: if ws.data[len(ws.data)-1] == '\xff' {
if err == nil {

[ukai, 2010-10-19 02:29:31]

http://codereview.appspot.com/2302042/diff/22001/src/pkg/websocket/websocket.go
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/2302042/diff/22001/src/pkg/websocket/websocket....
src/pkg/websocket/websocket.go:114: if ws.data[len(ws.data)-1] == '\xff' {
On 2010/10/18 15:13:43, rsc1 wrote:
> if err == nil {

Done.

[ukai, 2010-10-19 02:29:51]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-10-21 02:35:59]

LGTM

[rsc, 2010-10-21 02:36:12]

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

web socket: fix short Read

Fixes issue 1145.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2302042

Committer: Russ Cox <rsc@golang.org>