Skip to content

zlib: align with streams - #32220

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:zlib-align-w-streams2
Closed

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

Conversation

@ronag

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

@ronagronag added stream Issues and PRs related to the stream subsystem. zlib Issues and PRs related to the zlib subsystem. labels Mar 12, 2020
@ronag
ronag requested a review from addaleaxMarch 12, 2020 07:20
@ronag
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
ronagforce-pushed the zlib-align-w-streams2 branch from eb857ea to f9dfae6CompareMarch 12, 2020 07:21
@ronagronag mentioned this pull request Mar 12, 2020
Closed
4 tasks
ronag
ronag commented Mar 12, 2020
View reviewed changes
Comment threadlib/zlib.js
}

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

@ronagronagMar 12, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We could keep _hadError for compat if necessary.

@ronag
ronag requested a review from lpincaMarch 13, 2020 09:24
@ronag

ronag commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

@nodejs/zlib

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

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

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

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

@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 19, 2020
mcollina
mcollina reviewed Mar 19, 2020
View reviewed changes
Comment threadlib/zlib.js
Comment threadlib/zlib.js
mcollina
mcollina approved these changes Mar 19, 2020
View reviewed changes

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

lgtm

@nodejs-github-bot

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Mar 19, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in a940143

@ronagronag closed this Mar 19, 2020
ronag added a commit that referenced this pull request Mar 19, 2020
@ronag
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

ronag commented Mar 19, 2020

Copy link
Copy Markdown
MemberAuthor

FYI @BridgeAR

@MylesBorins

MylesBorins commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

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

@ronagronag 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
@ronag
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

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');newInflate().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.

@addaleaxaddaleax 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
@BethGriggsBethGriggs mentioned this pull request Mar 25, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeARBridgeARBridgeAR left review comments
@mcollinamcollinamcollina approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincaAwaiting requested review from lpinca

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.zlibIssues 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

@ronag@BridgeAR@nodejs-github-bot@MylesBorins@kevinoid@mcollina@addaleax