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

Fix "zlib binding closed" errors #632

Merged
merged 1 commit into from
Dec 21, 2015
Merged

Conversation

darrachequesne
Copy link
Contributor

This should fix #628, which I think is caused by:

  • deflate is closed
  • deflate.write() then triggers an 'error' event
  • cleanup() removes the listener attached to the 'error' event
  • deflate.flush() triggers the uncaught error 'zlib binding closed'

Does this make any sense?

@nkzawa
Copy link
Contributor

nkzawa commented Dec 16, 2015

👍 It might be better to add a guard to compress and decompress for checking if the instance is already cleaned up, like https://github.com/websockets/ws/blob/master/lib/Receiver.js#L269 .

@@ -76,10 +76,18 @@ PerMessageDeflate.prototype.accept = function(paramsList) {

PerMessageDeflate.prototype.cleanup = function() {
if (this._inflate) {
if (this._inflate.writeInProgress) {
this._inflate.pendingClose = true;
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this codebase, but... I think this means that if there is an _inflate.writeInProgress when the connection is closed, the _deflate will never be closed. Instead of returning, this should be rewritten as:

if (this._inflate.writeInProgress) {
      this._inflate.pendingClose = true;
} else {
      if (this._inflate.close) this._inflate.close();
     this._inflate = null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right! I'll patch this.

@rauchg
Copy link
Contributor

rauchg commented Dec 20, 2015

👍

Tested & works perfectly. No more

node: ../src/node_zlib.cc:132: static v8::Handle<v8::Value> node::ZCtx::Write(const v8::Arguments&): Assertion `!ctx->pending_close_ && "close is pending"' failed.
Aborted (core dumped)

@3rd-Eden
Copy link
Member

Adressing this tomorrow

Arnout Kazemier

On Dec 20, 2015, at 5:43 PM, Guillermo Rauch notifications@github.com wrote:

Tested & works perfectly. No more

node: ../src/node_zlib.cc:132: static v8::Handlev8::Value node::ZCtx::Write(const v8::Arguments&): Assertion `!ctx->pending_close_ && "close is pending"' failed.
Aborted (core dumped)

Reply to this email directly or view it on GitHub.

3rd-Eden added a commit that referenced this pull request Dec 21, 2015
Fix "zlib binding closed" errors
@3rd-Eden 3rd-Eden merged commit 4423b6e into websockets:master Dec 21, 2015
@darrachequesne
Copy link
Contributor Author

Thanks! That should fix #633 too.

@darrachequesne
Copy link
Contributor Author

Hi! @3rd-Eden could you please release this as version 0.8.2 before the big 1.0? 🙏 (the current 0.8.1 is not really usable due to that issue).

@3rd-Eden
Copy link
Member

Release of 1.0 goes out tomorrow. No 0.8.2 -- if you have issues, turning off deflate by default

Arnout Kazemier

On Dec 22, 2015, at 6:33 PM, Damien Arrachequesne notifications@github.com wrote:

Hi! @3rd-Eden could you please release this as version 0.8.2 before the big 1.0? (the current 0.8.1 is not really usable due to that issue).


Reply to this email directly or view it on GitHub.

deanlandolt added a commit to deanlandolt/websocket-stream that referenced this pull request Jan 4, 2016
Necessary to fix websockets/ws#632
knolleary added a commit to node-red/node-red that referenced this pull request Feb 10, 2016
Workaround for this issue: websockets/ws#632
as it has been fixed in the 1.x release that drops support for
node 0.10...
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 this pull request may close these issues.

Uncaught "zlib binding closed" errors
5 participants