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

zlib: emit "close" if Z_STREAM_END occurs. #3747

Closed
wants to merge 2 commits into from

Conversation

eungjun-yi
Copy link

In src/node_zlib.cc, Z_OK, Z_STREAM_END and Z_BUF_ERROR has been ignored because they are considered normal statuses.

But Z_STREAM_END must be handled. Even if zlib does not end when it meets Z_STREAM_END, it should at least tell user so that user can stop it.

(Actaully, I believe "end" is better than "close" for this case, but emitting "end" may cause exception because zlib does not allow to write something to itself if zlib._ended is true.)

@isaacs
Copy link

isaacs commented Jul 20, 2012

This isn't quite accurate.

Z_STREAM_END is the return value if all the output is consumed, and the flush param was set to Z_FINISH. However, the "close" event should be used only when the underlying resource is completely disposed of (ie, when deflateEnd or inflateEnd has been successfully called to free the memory.) This would occur when the binding has been deleted, but we don't actually track that, and just let that happen when V8's garbage collector cleans things up.

However, upon receiving a Z_STREAM_END, you can call .reset() on the stream, and then re-use it again. In fact, this is a useful way to prevent having to create new underlying objects.

What we probably ought to do is remove the place where we're currently emitting "end", and only emit "end" when a Z_STREAM_END return value is encountered. Would you like to change your patch to do that?

Emit "end" instead of "close" when Z_STREAM_END return value is
encountered. And remove emitting "end" in Zlib.end().
@eungjun-yi
Copy link
Author

You are right, @isaacs . I have done as you said and it looks to work well.

@Nodejs-Jenkins
Copy link

Can one of the admins verify this patch?

@indutny
Copy link
Member

indutny commented Feb 18, 2014

I guess it isn't relevant anymore?

@indutny indutny closed this Feb 18, 2014
kaiquewdev pushed a commit to kaiquewdev/node that referenced this pull request Nov 26, 2016
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs/node#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs/node#8169).
  * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs/node#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs/node#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs/node#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs/node#8317), [nodejs#8852](nodejs/node#8852), [nodejs#9253](nodejs/node#9253).
  * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs/node#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs/node#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs/node#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs/node#8217).
* Punycode
  * The `punycode` module has been deprecated [nodejs#7941](nodejs/node#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs/node#7448).

PR-URL: nodejs/node#9099
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants