Skip to content

stream: fix error-path function call - #41433

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
Trott:stream-error
Jan 11, 2022
Merged

stream: fix error-path function call#41433
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
Trott:stream-error

Conversation

@Trott

@TrottTrott commented Jan 7, 2022

Copy link
Copy Markdown
Member

The onFinish() function takes a single argument. The two extra
arguments passed here are already in the function scope, and may result
in the error being mishandled.

The `onFinish()` function takes a single argument. The two extra
arguments passed here are already in the function scope, and may result
in the error being mishandled.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 7, 2022
@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 7, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 7, 2022
@nodejs-github-bot

This comment has been minimized.

@apapirovskiapapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add a test so that this isn't so easy to break in the future? Looks like the error behavior has been broken for 6 months... Or at least open an issue ticket to add a test.

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@VoltrexKeyvaVoltrexKeyva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 8, 2022
@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 11, 2022
@Trott

Copy link
Copy Markdown
MemberAuthor

Should we add a test so that this isn't so easy to break in the future? Looks like the error behavior has been broken for 6 months... Or at least open an issue ticket to add a test.

Based on https://coverage.nodejs.org/coverage-f17b73cebd82babe/lib/internal/streams/writable.js.html#L715, it appears that the line of code in question here is not tested at all. 😳

@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 11, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 8c3637c into nodejs:masterJan 11, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 8c3637c

@Trott
Trott deleted the stream-error branch January 11, 2022 02:55
Trott added a commit to Trott/io.js that referenced this pull request Jan 12, 2022
targos pushed a commit that referenced this pull request Jan 14, 2022
The `onFinish()` function takes a single argument. The two extra
arguments passed here are already in the function scope, and may result
in the error being mishandled.
PR-URL: #41433
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@targostargos mentioned this pull request Jan 16, 2022
nodejs-github-bot pushed a commit that referenced this pull request Jan 17, 2022
Refs: #41433 (comment)
PR-URL: #41486
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Refs: #41433 (comment)
PR-URL: #41486
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
The `onFinish()` function takes a single argument. The two extra
arguments passed here are already in the function scope, and may result
in the error being mishandled.
PR-URL: #41433
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@Trott@nodejs-github-bot@apapirovski@mcollina@benjamingr@lpinca@ronag@VoltrexKeyva