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

zlib: allow writes after readable 'end' to finish by addaleax · Pull Request #31082 · nodejs/node · GitHub

/ node Public

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

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:zlib-finish-writes
Closed

zlib: allow writes after readable 'end' to finish#31082
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:zlib-finish-writes

Conversation

Copy link
Copy Markdown
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 added the zlib Issues and PRs related to the zlib subsystem. label Dec 24, 2019

nodejs-github-bot commented Dec 24, 2019
edited by addaleax
Loading

Copy link
Copy Markdown
Collaborator

Comment thread lib/zlib.js Outdated
Co-Authored-By: Denys Otrishko <9109612+lundibundi@users.noreply.github.com>
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 24, 2019

Copy link
Copy Markdown
Collaborator

lpinca commented Dec 25, 2019

Copy link
Copy Markdown
Member

Thank you Anna.

Comment thread lib/zlib.js Outdated
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member Author

Landed in 0e89b64

addaleax closed this Dec 27, 2019
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 commented Dec 27, 2019

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 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 mentioned this pull request Feb 8, 2020

Copy link
Copy Markdown

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.

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
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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL