Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

websocket was disconnect when [email protected] #5557

Closed
symplute opened this issue May 25, 2013 · 24 comments
Closed

websocket was disconnect when [email protected] #5557

symplute opened this issue May 25, 2013 · 24 comments

Comments

@symplute
Copy link

When I update from 0.10.7 to 0.10.8, websocket disconnect with warning on upgrade.

warn - 'websocket parser error: reserved fields must be empty'
info - 'transport end (undefined)'

My system use socket.io 0.9.14, express, node-spdy, ubuntu 13.04.

I change (revert) http.js#1959-1960 to use bodyHead instead of emptyBuffer, get not disconnect.

Both emptyBuffer and bodyHead is Buffer length = 0, Hmm... scope?

Please give me some advice.

@bnoordhuis
Copy link
Member

/cc @isaacs @nathan7

@edef1c
Copy link

edef1c commented May 26, 2013

Ahoy. This is an issue with the WebSocket parser used by socket.io.
It's making the unreasonable assumption that everything will arrive in the first chunk.

@edef1c
Copy link

edef1c commented May 26, 2013

/cc @3rd-Eden

@edef1c
Copy link

edef1c commented May 26, 2013

This could be a bug in a specific parser of ws.
All the ws tests pass except for the SSL-related ones (due to unrelated changes in SSL cert strictness)

@Zurgy
Copy link

Zurgy commented May 26, 2013

Fails for me when I use node-spdy like the original poster, works with http.

@edef1c
Copy link

edef1c commented May 26, 2013

@Zurgy Sweet, thanks.

@Zurgy
Copy link

Zurgy commented May 26, 2013

Oh, node 0.10.5 works fine. If I get a chance later I will try and investigate further.

@edef1c
Copy link

edef1c commented May 26, 2013

Seems socket.io 0.9.14 actually uses its own parser (there's an unrelated 0.9 branch for.. confusion?)

@Zurgy
Copy link

Zurgy commented May 26, 2013

Hmm, just enabled node-spdy in my dev environment and it works with node-0.10.8, socket.io 0-9.14, connect 2.7.10, spdy 1.5.0. Now need to figure out why it fails in my production environment with the same versions of these packages but does work in my prod env with node 0.10.5.
Disclaimer: this is a very rushed check as I'm about to go out.

@Zurgy
Copy link

Zurgy commented May 26, 2013

Node-0.10.7 works in my production environment where socket.io which fails with node-0.10.8.

@edef1c
Copy link

edef1c commented May 26, 2013

Okay, reproduced. Test case: https://gist.github.com/5652741

@edef1c
Copy link

edef1c commented May 26, 2013

I also managed to trigger node: ../deps/uv/src/unix/stream.c:726: uv__write: Assertion stream->write_queue_size == 0' failed.`, but it seems unrelated.

@edef1c
Copy link

edef1c commented May 26, 2013

It doesn't actually seem to be necessary to use SPDY. I disabled SPDY in the browser in the test case, and it's still reproducible.

@tmeasday
Copy link

@nathan7 - can you elaborate at all on "All the ws tests pass except for the SSL-related ones (due to unrelated changes in SSL cert strictness)"?

@edef1c
Copy link

edef1c commented May 26, 2013

@tmeasday All the tests of ws pass, except for about three of them which broke because node's TLS checks certificates by default. That should not influence real-world use.

@edef1c
Copy link

edef1c commented May 26, 2013

oortcloud/meteorite#135 shows that HTTPS is the causative factor. SPDY is not necessary to reproduce.
Test case updated to use just HTTPS.

@tmeasday
Copy link

So SSL is unrelated, and perhaps is just triggering the bug because it requires larger chunks? We are just using ws in a very straightforward way, no "specific parser" I don't think.

@bnoordhuis
Copy link
Member

I also managed to trigger node: ../deps/uv/src/unix/stream.c:726: uv__write: Assertion `stream->write_queue_size == 0' failed., but it seems unrelated.

It is (unrelated) and it's fixed in joyent/libuv@b38c9c1.

@edef1c
Copy link

edef1c commented May 27, 2013

This seems to go away if I make socket.io use streams2 for feeding data to the parser.

@silviapfeiffer
Copy link

I have the same problem on ubuntu with ssl - it will forever throw "reserved fields must be empty" on a ssl connection with v0.10.8. I don't use SPDY, just https.

@silviapfeiffer
Copy link

Just a note: I had a problem running my node server under root privileges (so I could access the ssl certificates). When I instead copied them somewhere accessible and ran the node server without root privileges, it worked again.

@edef1c
Copy link

edef1c commented May 27, 2013

I suspect this is something odd with streams2 compatibility. I haven't yet got a repro outside socket.io, I'll try to make my test case use ws today.

@symplute
Copy link
Author

When I read indutny/node-spdy source code,
I found 'socket.unshift(data);' in node-spdy/lib/spdy/server.js#200.

I get comment '.unshift() emits readable event#195'. Hmm...?
'.unshift()' was added in lib/http.js#1959.

I think that it has some relation.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jun 3, 2013
In some cases, the http CONNECT/Upgrade API is unshifting an empty
bodyHead buffer onto the socket.

Normally, stream.unshift(chunk) does not set state.reading=false.
However, this check was not being done for the case when the chunk was
empty (either `''` or `Buffer(0)`), and as a result, it was causing the
socket to think that a read had completed, and to stop providing data.

This bug is not limited to http or web sockets, but rather would affect
any parser that unshifts data back onto the source stream without being
very careful to never unshift an empty chunk.  Since the intent of
unshift is to *not* change the state.reading property, this is a bug.

Fixes nodejs#5557
Fixes socketio/socket.io#1242
@symplute
Copy link
Author

symplute commented Jun 5, 2013

It fixed and work with 0.10.10! I close this issue. Thank you for your support!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants