Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After first writeOrBuffer() state.writing remains true all the time #56

Closed
binarykitchen opened this issue Jan 12, 2015 · 13 comments · Fixed by #57
Closed

After first writeOrBuffer() state.writing remains true all the time #56

binarykitchen opened this issue Jan 12, 2015 · 13 comments · Fixed by #57

Comments

@binarykitchen
Copy link
Contributor

This is a tricky problem and I cannot locate the bug. Somehow, after upgrading to "websocket-stream": "^1.3.0", my client can write only once and then nothing happens.

After some debugging I found out that state.writing is always true after the first write operation. This in writeOrBuffer() of ./node_modules/websocket-stream/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js

  if (state.writing || state.corked)
    state.buffer.push(new WriteReq(chunk, encoding, cb));
  else
    doWrite(stream, state, false, len, chunk, encoding, cb);

In other words, after the first write operation, everything else is pushed in the buffer and nothing happens. Any clues?

@binarykitchen
Copy link
Contributor Author

Not sure, but it seems there is a mess with the different versions of the Writable class of the readable-stream packages in duplexify and through2.

@max-mapper
Copy link
Owner

Hmmm weird. I can confirm the bug. Not sure what is causing it either...

On Sun, Jan 11, 2015 at 7:33 PM, Michael Heuberger <notifications@github.com

wrote:

Not sure, but it seems there is a mess with the different versions of the
Writable class of the readable-stream packages in duplexify and through2.


Reply to this email directly or view it on GitHub
#56 (comment)
.

@binarykitchen
Copy link
Contributor Author

Right @maxogden ... any chance you could look into this and fix it pretty soon? It's breaking www.videomail.io :(

@max-mapper
Copy link
Owner

can't make any promises, for now I would recommend downgrading

On Sun, Jan 11, 2015 at 7:43 PM, Michael Heuberger <notifications@github.com

wrote:

Right @maxogden https://github.com/maxogden ... any chance you could
look into this and fix it pretty soon? It's breaking www.videomail.io :(


Reply to this email directly or view it on GitHub
#56 (comment)
.

@binarykitchen
Copy link
Contributor Author

I already tried to downgrade to v1.1 - no success

@binarykitchen
Copy link
Contributor Author

Forget my above comment, I double-checked, downgrading to v1.1 worked (had to fix an npm cache issue beforehand).

Then I verified that, when upgrading to v1.2, the bug is back! So the bug must have been introduced between the versions 1.1 and 1.2

Maybe one of you can explain? @mcollina @maxogden @3sv

@mcollina
Copy link
Collaborator

Sorry about that, I'll look into it asap: I definitely screw something up.
I am not exactly sure what's happening, as it passes all my MQTT.js
websocket tests.
Anyway, I have a strong suspect about this!

Does this happen in the Browser, right?
Il giorno lun 12 gen 2015 alle 07:21 Michael Heuberger <
notifications@github.com> ha scritto:

Forget my above comment, I double-checked, downgrading to v1.1 worked (had
to fix an npm cache issue beforehand).

Then I verified that, when upgrading to v1.2, the bug is back! So the bug
must have been introduced between the versions 1.1 and 1.2

Maybe one of you can explain? @mcollina https://github.com/mcollina
@maxogden https://github.com/maxogden @3sv https://github.com/3sv


Reply to this email directly or view it on GitHub
#56 (comment)
.

@binarykitchen
Copy link
Contributor Author

Great thanks. Yeah, happens in the browser (I am using it for www.videomail.io) ...

If you have eliminated the bug, please add a test case for that, thx!

@mcollina
Copy link
Collaborator

Can you please check if the just-released 1.3.0 fixes it? It might.
Il giorno lun 12 gen 2015 alle 07:49 Michael Heuberger <
notifications@github.com> ha scritto:

Great thanks. Yeah, happens in the browser (I am using it for
www.videomail.io) ...

If you have eliminated the bug, please add a test case for that, thx!


Reply to this email directly or view it on GitHub
#56 (comment)
.

@binarykitchen
Copy link
Contributor Author

Nope, v1.3 still has the bug. I already tested with v1.3 hours before. Bug is still here.

@georgzoeller
Copy link

Same here.

I'm using websocket-stream as a transport for a voxel-server/voxel-client and it fails transmitting anything from the client past the first packet with 1.2.0/1.3.0 in chrome 39.0.2171.95 m. Curiously, writes from the node server to the client seem to happen just fine.

Downgrading to 1.1.x is working fine.

mcollina added a commit that referenced this issue Jan 12, 2015
Do not use backpressure in browsers. Fixes #56.
@mcollina
Copy link
Collaborator

The fix is released as 1.3.1.

@binarykitchen
Copy link
Contributor Author

@mcollina Grazie mille!

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

Successfully merging a pull request may close this issue.

4 participants