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

zlib: align with streams by ronag · Pull Request #32220 · nodejs/node · GitHub

/ node Public

zlib: align with streams - #32220

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:zlib-align-w-streams2
ConversationCommits (3)ChecksFiles changed
Closed

zlib: align with streams#32220
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:zlib-align-w-streams2

Conversation

ronag commented Mar 12, 2020

Copy link
Copy Markdown
Member
  • Ensure automatic destruction only happens after both
    'end' and 'finish' has been emitted through autoDestroy.
  • Ensure close() callback is always invoked.
  • Ensure 'error' is only emitted once.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

ronag added stream Issues and PRs related to the stream subsystem. zlib Issues and PRs related to the zlib subsystem. labels Mar 12, 2020
ronag requested a review from addaleax March 12, 2020 07:20
zlib: align with streams
f9dfae6 
- Ensure automatic destruction only happens after both
'end' and 'finish' has been emitted through autoDestroy.
- Ensure close() callback is always invoked.
- Ensure 'error' is only emitted once.
ronag force-pushed the zlib-align-w-streams2 branch from eb857ea to f9dfae6 Compare March 12, 2020 07:21
ronag mentioned this pull request Mar 12, 2020
Closed
4 tasks
ronag commented Mar 12, 2020
View reviewed changes
Comment thread lib/zlib.js
}

Transform.call(this, { autoDestroy: false, ...opts });
this._hadError = false;

ronag Mar 12, 2020

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

We could keep _hadError for compat if necessary.

ronag requested a review from lpinca March 13, 2020 09:24

ronag commented Mar 13, 2020

Copy link
Copy Markdown
Member Author

@nodejs/zlib

ronag requested a review from mcollina March 13, 2020 09:25
mcollina reviewed Mar 13, 2020
View reviewed changes
Comment thread lib/zlib.js Outdated
ronag mentioned this pull request Mar 14, 2020
Closed
BridgeAR requested a review from mcollina March 17, 2020 00:41
BridgeAR reviewed Mar 17, 2020
View reviewed changes
Comment thread lib/zlib.js Outdated

BridgeAR commented Mar 17, 2020

Copy link
Copy Markdown
Member

It would be good to get another review. I would like to get this into the next v13.x release, if this is ready.

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29916/

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 19, 2020
mcollina reviewed Mar 19, 2020
View reviewed changes
Comment thread lib/zlib.js
Comment thread lib/zlib.js
mcollina approved these changes Mar 19, 2020
View reviewed changes

mcollina left a comment

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

lgtm

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29920/

ronag commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

Landed in a940143

ronag closed this Mar 19, 2020
ronag added a commit that referenced this pull request Mar 19, 2020
zlib: align with streams
a940143 
- Ensure automatic destruction only happens after both
'end' and 'finish' has been emitted through autoDestroy.
- Ensure close() callback is always invoked.
- Ensure 'error' is only emitted once.

PR-URL: #32220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ronag commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

FYI @BridgeAR

MylesBorins commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

Hey @ronag, this doesn't land cleanly on v13.x, should it be backported?

ronag mentioned this pull request Mar 19, 2020
Closed
4 tasks
ronag added a commit to nxtedition/node that referenced this pull request Mar 19, 2020
zlib: align with streams
76fd0e5 
- Ensure automatic destruction only happens after both
'end' and 'finish' has been emitted through autoDestroy.
- Ensure close() callback is always invoked.
- Ensure 'error' is only emitted once.

PR-URL: nodejs#32220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Backport-PR-URL: nodejs#32371

kevinoid commented Mar 23, 2020

Copy link
Copy Markdown
Contributor

It appears that as a result of this PR, close passes ERR_STREAM_PREMATURE_CLOSE to its callback when closed before 'end'. For example:

const { Inflate } = require('zlib');
new Inflate().close(console.error);

Prints nothing in 543c046 and ERR_STREAM_PREMATURE_CLOSE in a940143 (and current master, 668bc11).

I don't know whether this is expected behavior and/or whether the docs should be clearer (either about when to call close or what errors to expect), but thought it was worth mentioning to be sure you are aware of the change.

addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed backport-requested-v13.x labels Mar 23, 2020
BethGriggs mentioned this pull request Mar 25, 2020
Merged
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

BridgeAR BridgeAR left review comments
mcollina mcollina approved these changes
addaleax addaleax approved these changes
lpinca Awaiting requested review from lpinca

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. 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.

7 participants

Back | FazBrowse Home | New Git URL