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

[v14.x backport] stream: fix multiple Writable.destroy() calls #38473

Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 29, 2021

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

@github-actions github-actions bot added needs-ci PRs that need a full CI run. v14.x labels Apr 29, 2021
ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
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
@nodejs-github-bot
Copy link
Collaborator

ronag added a commit to nxtedition/node that referenced this pull request Apr 29, 2021
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
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
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag removed the needs-ci PRs that need a full CI run. label Apr 29, 2021
@ronag
Copy link
Member Author

ronag commented Apr 29, 2021

Not sure if there really is anything to backport here...

@targos
Copy link
Member

targos commented Apr 30, 2021

Not sure if there really is anything to backport here...

What do you mean? Does it cherry-pick without conflicts?

@ronag
Copy link
Member Author

ronag commented Apr 30, 2021

What do you mean? Does it cherry-pick without conflicts?

I mean that the only thing to backport is the test. The bug the commit fixes doesn't exist on 14.x.

@targos
Copy link
Member

targos commented Apr 30, 2021

Ah, I see. I think the backport was requested because of the lts-watch-v14.x label.

@targos
Copy link
Member

targos commented Apr 30, 2021

Do you know which commit/PR introduced the bug?

@danielleadams
Copy link
Member

Ah yes, it had the tag and when I tried to backport it didn't land cleanly. If there's nothing to backport, I guess we can just land the test if it covers the previous bug?

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

targos pushed a commit that referenced this pull request May 30, 2021
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
Copy link
Member

targos commented May 30, 2021

Landed in 358ae8b

@targos targos closed this May 30, 2021
targos pushed a commit that referenced this pull request Jun 5, 2021
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
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 11, 2021
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants