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

[v13.x backport] zlib: align with streams by ronag · Pull Request #32371 · nodejs/node · GitHub

/ node Public

[v13.x backport] zlib: align with streams - #32371

Closed
ronag wants to merge 2 commits into
nodejs:v13.x-stagingfrom
nxtedition:backport-32220-to-v13.x
ConversationCommits (2)ChecksFiles changed
Closed

[v13.x backport] zlib: align with streams#32371
ronag wants to merge 2 commits into
nodejs:v13.x-stagingfrom
nxtedition:backport-32220-to-v13.x

Conversation

ronag commented Mar 19, 2020

Copy link
Copy Markdown
Member

PR-URL: #32220

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

nodejs-github-bot added v13.x zlib Issues and PRs related to the zlib subsystem. labels Mar 19, 2020
ronag requested review from BridgeAR and MylesBorins March 19, 2020 21:11

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

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

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
ronag force-pushed the backport-32220-to-v13.x branch from dc563f1 to 76fd0e5 Compare March 19, 2020 21:15

MylesBorins commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

@ronag does #32050 still need to land or should that be marked "do-not-land" now?

MylesBorins commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

also there are a ton of failures PTAL

ronag commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

@ronag does #32050 still need to land or should that be marked "do-not-land" now?

do not land

ronag commented Mar 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

I think most of the failures is due to the fact we don't emit 'error' twice but I'd like a second opinion. @nodejs/streams. See the fixup commit.

nodejs-github-bot commented Mar 19, 2020

Copy link
Copy Markdown
Collaborator

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

ronag commented Mar 19, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-destroy.js
ts.on('close', common.mustCall(() => {
ts.close(common.mustCall());
}));
ts.on('close', common.mustCall());

ronag Mar 19, 2020 •
edited
Loading

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

finished does not invoke callback on closed stream, this was a semver-major fix does not land on 13

ronag commented Mar 19, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-write-after-close.js
name: 'Error',
message: 'Cannot call write after a stream was destroyed'
}
);

ronag Mar 19, 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

I'm a little unsure how exactly this could throw before.

ronag commented Mar 19, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-object-write.js
assert.throws(
() => gunzip.write({}),
TypeError
);

ronag Mar 19, 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

I'm a little unsure how exactly this could throw before.

ronag commented Mar 23, 2020

Copy link
Copy Markdown
Member Author

PR was just labeled as semver-major so I guess this should not be backported.

ronag closed this Mar 23, 2020
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 Awaiting requested review from BridgeAR
MylesBorins Awaiting requested review from MylesBorins

Assignees

No one assigned

Labels

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.

3 participants

Back | FazBrowse Home | New Git URL