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

zlib: allow writes after readable 'end' to finish #31082

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call .destroy()
on the stream in .on('end'), and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
_write() callback would never have been called.

/cc @lpinca

Fixes: #30976

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: nodejs#30976
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Dec 24, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 24, 2019

lib/zlib.js Outdated Show resolved Hide resolved
Co-Authored-By: Denys Otrishko <9109612+lundibundi@users.noreply.github.com>
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Dec 25, 2019

Thank you Anna.

lib/zlib.js Outdated Show resolved Hide resolved
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

Landed in 0e89b64

@addaleax addaleax closed this Dec 27, 2019
@addaleax addaleax deleted the zlib-finish-writes branch December 27, 2019 01:14
addaleax added a commit that referenced this pull request Dec 27, 2019
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@lpinca
Copy link
Member

lpinca commented Dec 27, 2019

I think this should not land on v10.x unless 28db96f is also backported. Please correct me if I am wrong.

@addaleax
Copy link
Member Author

@lpinca I don’t think there’s a reason to not land this on v10.x and make behaviour more consistent across versions, even if 28db96f doesn’t land

BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@stefanpenner
Copy link

Just an FYI, it appears (after bisect) this commit yarn pack no longer invoked it's prepack hook. issue I believe though, that root cause is some unsafe stream code in yarn PR Fixing but I wanted to share here, in-case I got it wrong or this was unintended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zlib deflate/inflate failure