Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 19, 2020

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 nodejs-github-bot added v13.x zlib Issues and PRs related to the zlib subsystem. labels Mar 19, 2020
@ronag ronag requested review from BridgeAR and MylesBorins March 19, 2020 21:11
@nodejs-github-bot
Copy link
Collaborator

- 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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Backport-PR-URL: nodejs#32371
@ronag ronag force-pushed the backport-32220-to-v13.x branch from dc563f1 to 76fd0e5 Compare March 19, 2020 21:15
@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

also there are a ton of failures PTAL

@ronag
Copy link
Member Author

ronag commented Mar 19, 2020

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

do not land

@ronag
Copy link
Member Author

ronag commented Mar 19, 2020

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
Copy link
Collaborator

ts.on('close', common.mustCall(() => {
ts.close(common.mustCall());
}));
ts.on('close', common.mustCall());
Copy link
Member Author

@ronag ronag Mar 19, 2020

Choose a reason for hiding this comment

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

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

name: 'Error',
message: 'Cannot call write after a stream was destroyed'
}
);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

assert.throws(
() => gunzip.write({}),
TypeError
);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@ronag
Copy link
Member Author

ronag commented Mar 23, 2020

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

@ronag ronag closed this Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants