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.

Sign up for GitHub

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

Jump to bottom

stream: fix multiple Writable.destroy() calls. #38221

Closed
wants to merge 3 commits into from
Closed

stream: fix multiple Writable.destroy() calls. #38221

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 12, 2021

Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

@ronag
stream: fix multiple Writable.destroy() calls.
6ae4e4a 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 12, 2021
@ronag ronag requested review from mcollina and lpinca April 12, 2021 22:56
@ronag
Copy link
Member Author

ronag commented Apr 12, 2021

@nodejs/streams

mcollina
mcollina approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member

@Linkgoron

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Apr 14, 2021

@ronag ... looks like this has a linter issue

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2021
@ronag
Copy link
Member Author

ronag commented Apr 14, 2021

Just needs another CI run.

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Apr 16, 2021

Landed in 369f239

@ronag ronag closed this Apr 16, 2021
ronag added a commit that referenced this pull request Apr 16, 2021
@ronag
stream: fix multiple Writable.destroy() calls
369f239 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

PR-URL: #38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 20, 2021
Merged
@danielleadams
Copy link
Contributor

@ronag do you mind opening a Backport PR for this for v14.x-staging? The test-writable-stream-destroy tests when I try to pull it in. Thank you!

ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
@ronag
stream: fix multiple Writable.destroy() calls
6531b98 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189

PR-URL: nodejs#38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag ronag mentioned this pull request Apr 29, 2021
Closed
ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
@ronag
stream: fix multiple Writable.destroy() calls
a540afd 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189

PR-URL: nodejs#38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
@ronag
stream: fix multiple Writable.destroy() calls
33424e6 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189

PR-URL: nodejs#38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: nodejs#38473
@ronag
Copy link
Member Author

ronag commented Apr 29, 2021

@danielleadams #38473

ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
@ronag
stream: fix multiple Writable.destroy() calls
fa4b816 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189

PR-URL: nodejs#38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: nodejs#38473
ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
@ronag
stream: fix multiple Writable.destroy() calls
e516c0a 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: nodejs#38189

PR-URL: nodejs#38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: nodejs#38473
targos pushed a commit that referenced this pull request May 30, 2021
@ronag @targos
stream: fix multiple Writable.destroy() calls
358ae8b 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

PR-URL: #38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #38473
targos pushed a commit that referenced this pull request Jun 5, 2021
@ronag @targos
stream: fix multiple Writable.destroy() calls
41c62ca 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

PR-URL: #38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #38473
targos pushed a commit that referenced this pull request Jun 5, 2021
@ronag @targos
stream: fix multiple Writable.destroy() calls
152aa84 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

PR-URL: #38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #38473
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@ronag @targos
stream: fix multiple Writable.destroy() calls
dbb0d26 
Calling Writable.destroy() multiple times in the same tick
could cause an assertion error.

Fixes: #38189

PR-URL: #38221
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #38473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@Linkgoron Linkgoron Linkgoron approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reproductible ERR_INTERNAL_ASSERTION on node 15
10 participants
@ronag @nodejs-github-bot @benjamingr @jasnell @danielleadams @mcollina @Trott @Linkgoron @lpinca @targos