stream,zlib: do not use _stream_* anymore. by mcollina · Pull Request #36618 · nodejs/node

mcollina · 2020-12-24T15:29:15Z

In two place we still used _stream_*, causing #36615 and potentially other issues.

Note that #36615 does not impact master for some independent reason.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-12-24T15:45:06Z

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

mcollina · 2020-12-24T17:41:26Z

@nodejs/releasers @nodejs/tsc this should go to into one of the next v14 releases.

richardlau

LGTM.

Do we need to add a regression test to catch the breakage (I'm assuming no existing test caught #36615)?

mcollina · 2020-12-26T18:00:07Z

@richardlau I've added a test

targos · 2020-12-27T12:54:00Z

@mcollina can you fix the lint error?

mcollina · 2020-12-27T22:31:51Z

@targos done

nodejs-github-bot · 2020-12-27T22:37:00Z

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

nodejs-github-bot · 2020-12-28T08:37:32Z

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

github-actions · 2020-12-28T19:16:44Z

Commit Queue failed

- Loading data for nodejs/node/pull/36618
✔ Done loading data for nodejs/node/pull/36618
----------------------------------- PR info ------------------------------------
Title stream,zlib: do not use _stream_* anymore. (#36618)
Author Matteo Collina (@mcollina)
Branch mcollina:always-use-stream -> nodejs:master
Labels dont-land-on-v10.x, dont-land-on-v12.x, lts-watch-v14.x, zlib
Commits 1
- stream,zlib: do not use _stream_* anymore.
Committers 1
- Matteo Collina PR-URL: https://github.com/nodejs/node/pull/36618
Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36618
Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso --------------------------------------------------------------------------------
⚠ Commits were pushed since the last review:
⚠ - stream,zlib: do not use _stream_* anymore.
✔ Last GitHub Actions successful
ℹ Last Full PR CI on 2020-12-28T17:53:03Z: https://ci.nodejs.org/job/node-test-pull-request/35126/
- Querying data for job/node-test-pull-request/35126/
✔ Build data downloaded
✔ Last Jenkins CI successful
ℹ This PR was created on Thu, 24 Dec 2020 15:29:15 GMT
✔ Approvals: 6
✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36618#pullrequestreview-558672980
✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-558684130
✔ - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/36618#pullrequestreview-558965185
✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36618#pullrequestreview-558695858
✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36618#pullrequestreview-558712700
✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-559085773
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/449376978

mcollina · 2020-12-28T19:23:14Z

May I get a last approval @targos?

github-actions · 2020-12-28T22:36:57Z

Commit Queue failed

- Loading data for nodejs/node/pull/36618 ✔ Done loading data for nodejs/node/pull/36618 ----------------------------------- PR info ------------------------------------ Title stream,zlib: do not use _stream_* anymore. (#36618) Author Matteo Collina (@mcollina) Branch mcollina:always-use-stream -> nodejs:master Labels dont-land-on-v10.x, dont-land-on-v12.x, lts-watch-v14.x, zlib Commits 1 - stream,zlib: do not use _stream_* anymore. Committers 1 - Matteo Collina PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-12-28T19:16:44Z: https://ci.nodejs.org/job/node-test-pull-request/35126/ - Querying data for job/node-test-pull-request/35126/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Thu, 24 Dec 2020 15:29:15 GMT ✔ Approvals: 6 ✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36618#pullrequestreview-558672980 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-558684130 ✔ - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/36618#pullrequestreview-558965185 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36618#pullrequestreview-558695858 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36618#pullrequestreview-558712700 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-559319170 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 36618 From https://github.com/nodejs/node * branch refs/pull/36618/merge -> FETCH_HEAD ✔ Fetched commits as 053874939a60..be22d247cfc8 -------------------------------------------------------------------------------- [master ec57bf8288] stream,zlib: do not use _stream_* anymore. Author: Matteo Collina Date: Thu Dec 24 16:25:53 2020 +0100 3 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-zlib-no-stream.js ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- stream,zlib: do not use _stream_* anymore.

PR-URL: #36618
Reviewed-By: Robert Nagy ronagy@icloud.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Richard Lau rlau@redhat.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Michaël Zasso targos@protonmail.com

[master 83c0916250] stream,zlib: do not use *stream** anymore.
Author: Matteo Collina hello@matteocollina.com
Date: Thu Dec 24 16:25:53 2020 +0100
3 files changed, 16 insertions(+), 3 deletions(-)
create mode 100644 test/parallel/test-zlib-no-stream.js
✖ 83c0916250fb078d45eff18e06b7159eff4f69e4
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✖ 0:42 Do not use punctuation at end of title. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/449646494

PR-URL: nodejs#36618 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

aduh95 · 2020-12-29T00:48:56Z

Landed in e57d8af

PR-URL: #36618 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Notable changes: - **deps**: upgrade npm to 6.14.11 (Darcy Clarke) [#36838](#36838) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) [#36618](#36618) PR-URL: #37074

PR-URL: #36618 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Notable changes: - **deps**: upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074

Notable changes: - **deps**: - upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - V8: backport dfcf1e86fac0 (Michaël Zasso) (#37245) - Note: Node.js is not believed to be vulnerable to CVE-2021-21148. - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074

mcollina requested a review from ronag December 24, 2020 15:29

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Dec 24, 2020

mcollina added dont-land-on-v10.x request-ci Add this label to start a Jenkins CI on a PR. labels Dec 24, 2020

This was referenced Dec 24, 2020
stream: move to internal/streams #35239
Closed
gunzipSync DOA in Node 14.15.2 #36615
Closed

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020

ronag approved these changes Dec 24, 2020
View reviewed changes

Trott approved these changes Dec 24, 2020
View reviewed changes

richardlau approved these changes Dec 24, 2020
View reviewed changes

lpinca approved these changes Dec 24, 2020
View reviewed changes

benjamingr approved these changes Dec 24, 2020
View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

richardlau approved these changes Dec 27, 2020
View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020

targos approved these changes Dec 28, 2020
View reviewed changes

Comment threadtest/parallel/test-zlib-no-stream.js Outdated

targos linked an issue Dec 28, 2020 that may be closed by this pull request
gunzipSync DOA in Node 14.15.2 #36615
Closed

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020

targos approved these changes Dec 28, 2020
View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 28, 2020

github-actionsBot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2020

aduh95 force-pushed the always-use-stream branch from be22d24 to e57d8afCompare December 29, 2020 00:48

aduh95 merged commit e57d8af into nodejs:masterDec 29, 2020

danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged

richardlau mentioned this pull request Jan 22, 2021
zlib deflateraw crash in readable stream in 14.15.2+ #37021
Closed

This was referenced Jan 22, 2021
DeflateCRC32Stream is broken with NodeJS 15.6.0 archiverjs/node-crc32-stream#31
Closed
Node v15.6.0 changes the behavior of .end on zlib.DeflateRaw#37027
Closed

BethGriggs removed the lts-watch-v14.x label Jan 26, 2021

BethGriggs mentioned this pull request Jan 26, 2021
v14.15.5 proposal #37074
Merged

BethGriggs added a commit that referenced this pull request Feb 5, 2021
2021-02-08, Version 14.15.5 'Fermium' (LTS)
a1b41b3
Notable changes: - **deps**: upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074

RoyHP mentioned this pull request Feb 10, 2021
Node silently exits with code 0, no error, when using a specific string for String.includes inside worker piscinajs/piscina#107
Closed

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Uh oh!

Uh oh!

Conversation

mcollina commented Dec 24, 2020

Checklist

Uh oh!

nodejs-github-bot commented Dec 24, 2020

Uh oh!

mcollina commented Dec 24, 2020

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 26, 2020

Uh oh!

targos commented Dec 27, 2020

Uh oh!

mcollina commented Dec 27, 2020

Uh oh!

nodejs-github-bot commented Dec 27, 2020

Uh oh!

nodejs-github-bot commented Dec 28, 2020

Uh oh!

Uh oh!

github-actionsBot commented Dec 28, 2020

Uh oh!

mcollina commented Dec 28, 2020

Uh oh!

github-actionsBot commented Dec 28, 2020

PR-URL: #36618 Reviewed-By: Robert Nagy ronagy@icloud.com Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Richard Lau rlau@redhat.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Michaël Zasso targos@protonmail.com

Uh oh!

aduh95 commented Dec 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

PR-URL: #36618
Reviewed-By: Robert Nagy ronagy@icloud.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Richard Lau rlau@redhat.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Michaël Zasso targos@protonmail.com