Skip to content

stream: writable don't throw multiple callback error - #30614

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:stream-writable-err-multiple-callback
Closed

ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:stream-writable-err-multiple-callback

Conversation

@ronag

@ronag ronag commented Nov 23, 2019 •

Copy link
Copy Markdown
Member

Align ERR_MULTIPLE_CALLBACK in Writable with the rest of error handling as well as how the error is implemented in Transform.

I've just basically copied the Transform test and made sure Writable matches it.

Throwing here is dangerous and can lead to unnecessary unhandled exception.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Align ERR_MULTIPLE_CALLBACK in Writable with the rest
of error handling as well as how the error is implemented
in Transform.
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to Node.js streams. label Nov 23, 2019
@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 23, 2019
Comment thread lib/net.js Outdated
@ronag
ronag force-pushed the stream-writable-err-multiple-callback branch from 80813ce to e5c3e85 Compare November 24, 2019 09:54
@lpinca

lpinca commented Nov 24, 2019

Copy link
Copy Markdown
Member

Can you fix the test filename (test-stream-writable-callback-twice copy.js)?

@ronag

ronag commented Nov 24, 2019

Copy link
Copy Markdown
Member Author

@lpinca: Yep, sorry. I'll try to be a bit more diligent.

@ronag
ronag requested a review from lpinca November 24, 2019 10:21
@ronag

ronag commented Dec 1, 2019

Copy link
Copy Markdown
Member Author

@Trott: I believe this can land?

@lpinca

lpinca commented Dec 1, 2019

Copy link
Copy Markdown
Member

It needs at least one more TSC approval as it is semver-major.

cc: @nodejs/tsc

@ronag

ronag commented Dec 1, 2019

Copy link
Copy Markdown
Member Author

Ah, sorry. I thought @lpinca was TSC. My mistake.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Trott

Trott commented Dec 1, 2019

Copy link
Copy Markdown
Member

@ronag

ronag commented Dec 11, 2019 •

Copy link
Copy Markdown
Member Author

@Trott: This has 2 TSC approvals and should be able to land? Anything in the CITGM?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Trott

Trott commented Dec 14, 2019

Copy link
Copy Markdown
Member

CITGM looks good.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina mcollina 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

Trott pushed a commit that referenced this pull request Dec 14, 2019
Align ERR_MULTIPLE_CALLBACK in Writable with the rest
of error handling as well as how the error is implemented
in Transform.

PR-URL: #30614
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott

Trott commented Dec 14, 2019

Copy link
Copy Markdown
Member

Landed in e902fad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants