FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

zlib: Make the finish flush flag configurable by addaleax · Pull Request #6069 · nodejs/node · GitHub

/ node Public

zlib: Make the finish flush flag configurable - #6069

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:zlib-configurable-finish-flush
ConversationCommits (2)ChecksFiles changed
Closed

zlib: Make the finish flush flag configurable#6069
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:zlib-configurable-finish-flush

Conversation

addaleax commented Apr 5, 2016

Copy link
Copy Markdown
Member

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

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
Comment thread test/parallel/test-zlib-truncated.js Outdated

r-52 Apr 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(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)

addaleax Apr 6, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 Updated the PR with some tests for that.

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

r-52 commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

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

jasnell reviewed Apr 7, 2016
View reviewed changes
Comment thread doc/api/zlib.markdown Outdated

jasnell Apr 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 7, 2016

Copy link
Copy Markdown
Member

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

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

addaleax commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

@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.

zlib: Make the finish flush flag configurable
35ca9ad 
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
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
Comment thread doc/api/zlib.markdown Outdated

jasnell Apr 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 7, 2016

Copy link
Copy Markdown
Member

Looking better! Couple more comments but other than that LGTM

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

addaleax commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

LGTM
Thank you!

addaleax added a commit that referenced this pull request Apr 17, 2016
zlib: Make the finish flush flag configurable
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 <jasnell@gmail.com>

addaleax commented Apr 17, 2016

Copy link
Copy Markdown
Member Author

Landed in 9781667.

addaleax closed this Apr 17, 2016
addaleax deleted the zlib-configurable-finish-flush branch April 17, 2016 15:29
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
zlib: Make the finish flush flag configurable
83bc0a2 
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 <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
e43735b 
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
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
zlib: Make the finish flush flag configurable
2ac14ee 
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 <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-21, Version 5.11.0 (Stable) Release
8dc1d5e 
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
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
0c5ab5c 
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
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
1fa8fce 
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
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
zlib: Make the finish flush flag configurable
6c67613 
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 <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
2016-04-20, Version 5.11.0 (Stable) Release
002d57b 
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
jasnell pushed a commit that referenced this pull request Apr 26, 2016
zlib: Make the finish flush flag configurable
5f11b53 
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 <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
2016-04-20, Version 5.11.0 (Stable) Release
c764c44 
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
justinsg mentioned this pull request Jun 23, 2016
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minor PRs that contain new features and should be released in the next minor version. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gzip decoder too strict

5 participants

Back | FazBrowse Home | New Git URL