zlib: Make the finish flush flag configurable #6069

addaleax · 2016-04-05T23:01:47Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

zlib

Description of change

Up to now, Z_FINISH was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using Z_SYNC_FLUSH, if that suits their needs.

This was requested in #4030 and #5761.

Fixes: #5761

r-52 · 2016-04-06T12:48:02Z

test/parallel/test-zlib-truncated.js

+    const syncFlushOpt = { finishFlush: zlib.Z_SYNC_FLUSH };
+
+    // sync truncated input test, finishFlush = Z_SYNC_FLUSH
+    assert.doesNotThrow(function() {


(Just a suggestion/ idea): Maybe you could add a case that triggers the new error if the flush flag is not valid? (Just to make sure that the flag was caught somewhere)

👍 Updated the PR with some tests for that.

r-52 · 2016-04-07T13:59:09Z

And here is a CI run: https://ci.nodejs.org/job/node-test-pull-request/2197/

jasnell · 2016-04-07T17:13:57Z

doc/api/zlib.markdown

@@ -241,6 +242,10 @@ ignored by the decompression classes.
 See the description of `deflateInit2` and `inflateInit2` at
 <http://zlib.net/manual.html#Advanced> for more information on these.

+By default, the zlib methods will throw an error when decompressing truncated
+data. If you know what you are doing, you can override this behaviour by passing


The If you know what you are doing... part is a bit concerning here. Rather than this, perhaps include an example of using Z_SYNC_FLUSH and provide some detail about why someone would do so.

jasnell · 2016-04-07T17:14:16Z

The specific code change and tests LGTM but I think the doc updates need a bit more work.

addaleax · 2016-04-07T20:16:17Z

@jasnell I updated this with a full example for setting finishFlush. I would actually be fine with not documenting this beyond a mention in the Options section, as it is with the other “advanced” features (e.g. .flush, .dictionary).
I think you’re right in criticizing my initial wording, but I’d like to be careful in that the docs don’t come over as giving a general “this will fix your error” recipe. This feature is something that should be available, but should also be reserved for situations in which the user knows that they are silencing errors that are otherwise emitted for a good reason.

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: nodejs#5761

jasnell · 2016-04-07T21:57:32Z

doc/api/zlib.markdown

+By default, the zlib methods will throw an error when decompressing truncated
+data. If you know that your input may be incomplete, or you want to
+inspect only the beginning of a compressed file, you can change the flushing
+method which is used to compressed the last chunk of input data:


Better :-) ... I'd like to avoid using you, however. Perhaps:

By default, the zlib methods with throw an error when decompressing truncated data. However, if it is known that the data is incomplete, or the desire is to inspect only the beginning of a compressed file, it is possible to suppress the default error handling by changing the flushing method that is used:

jasnell · 2016-04-07T22:00:25Z

Looking better! Couple more comments but other than that LGTM

addaleax · 2016-04-07T22:52:53Z

👍 Updated the docs example again based on @jasnell’s suggestions.

jasnell · 2016-04-07T23:50:31Z

LGTM
Thank you!

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: #5761 PR-URL: #6069 Reviewed-By: James M Snell <[email protected]>

addaleax · 2016-04-17T15:29:42Z

Landed in 9781667.

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: #5761 PR-URL: #6069 Reviewed-By: James M Snell <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: #5761 PR-URL: #6069 Reviewed-By: James M Snell <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behavior was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: nodejs#5761 PR-URL: nodejs#6069 Reviewed-By: James M Snell <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) nodejs#5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) nodejs#6279 * update ESLint to 2.7.0 (silverwind) nodejs#6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) nodejs#6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) nodejs#6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) nodejs#6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) nodejs#6090 src: * add SIGINFO to supported signals (James Reggio) nodejs#6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) nodejs#6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) nodejs#6069 PR-URL: nodejs#6322

Up to now, `Z_FINISH` was always the flushing flag that was used for the last chunk of input data. This patch makes this choice configurable so that advanced users can perform e.g. decompression of partial data using `Z_SYNC_FLUSH`, if that suits their needs. Add tests to make sure that an error is thrown upon encountering invalid `flush` or `finishFlush` flags. Fixes: #5761 PR-URL: #6069 Reviewed-By: James M Snell <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Apr 5, 2016

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 5, 2016

addaleax force-pushed the zlib-configurable-finish-flush branch from 71ae4c7 to e92b676 Compare April 5, 2016 23:45

r-52 reviewed Apr 6, 2016
View reviewed changes

addaleax force-pushed the zlib-configurable-finish-flush branch from e92b676 to 3310d3c Compare April 6, 2016 13:11

jasnell reviewed Apr 7, 2016
View reviewed changes

addaleax force-pushed the zlib-configurable-finish-flush branch from 3310d3c to df28e72 Compare April 7, 2016 20:16

addaleax force-pushed the zlib-configurable-finish-flush branch from df28e72 to 8692667 Compare April 7, 2016 20:28

jasnell reviewed Apr 7, 2016
View reviewed changes

doc: Add full example for finishFlush to zlib docs

f85f54c

addaleax force-pushed the zlib-configurable-finish-flush branch from 8692667 to f85f54c Compare April 7, 2016 22:52

addaleax closed this Apr 17, 2016

addaleax deleted the zlib-configurable-finish-flush branch April 17, 2016 15:29

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

justinsg mentioned this pull request Jun 23, 2016

Is there a proper way to send zlib partial files for unzipping? #4030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: Make the finish flush flag configurable #6069

zlib: Make the finish flush flag configurable #6069

addaleax commented Apr 5, 2016

r-52 Apr 6, 2016

addaleax Apr 6, 2016

r-52 commented Apr 7, 2016

jasnell Apr 7, 2016

jasnell commented Apr 7, 2016

addaleax commented Apr 7, 2016

jasnell Apr 7, 2016

jasnell commented Apr 7, 2016

addaleax commented Apr 7, 2016

jasnell commented Apr 7, 2016

addaleax commented Apr 17, 2016

zlib: Make the finish flush flag configurable #6069

zlib: Make the finish flush flag configurable #6069

Conversation

addaleax commented Apr 5, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

r-52 Apr 6, 2016

Choose a reason for hiding this comment

addaleax Apr 6, 2016

Choose a reason for hiding this comment

r-52 commented Apr 7, 2016

jasnell Apr 7, 2016

Choose a reason for hiding this comment

jasnell commented Apr 7, 2016

addaleax commented Apr 7, 2016

jasnell Apr 7, 2016

Choose a reason for hiding this comment

jasnell commented Apr 7, 2016

addaleax commented Apr 7, 2016

jasnell commented Apr 7, 2016

addaleax commented Apr 17, 2016