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: improve multiple callback error message #12520

Merged
merged 1 commit into from
May 22, 2017
Merged

stream: improve multiple callback error message #12520

merged 1 commit into from
May 22, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 19, 2017

When a transform stream's callback is called more than once, an error is emitted with a somewhat confusing message. This commit hopes to improve the quality of the error message.

Fixes: #12513

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Apr 19, 2017
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 19, 2017
jasnell
jasnell reviewed Apr 19, 2017
View reviewed changes
lib/_stream_transform.js Outdated
function emitMultipleCallsError() {
this.emit('error', new Error('write callback called multiple times'));
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this, but it may make sense at some point to make this a utility in internal/util given that there may be several places where we'd want to avoid calling a callback more than once

cjihrig
cjihrig commented Apr 19, 2017
View reviewed changes
lib/_stream_transform.js Outdated
@@ -92,7 +96,7 @@ function afterTransform(stream, er, data) {
return stream.emit('error', new Error('no writecb in Transform class'));

ts.writechunk = null;
ts.writecb = null;
ts.writecb = emitMultipleCallsError.bind(stream);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note. If this PR gets accepted, this bind() could be moved to, for example, the stream constructor, and hidden with a symbol. That would avoid rebinding every time.

kt3k
kt3k reviewed Apr 20, 2017
View reviewed changes
lib/_stream_transform.js Outdated
@@ -92,7 +96,7 @@ function afterTransform(stream, er, data) {
return stream.emit('error', new Error('no writecb in Transform class'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this line useless if you assign non-null value to ts.writecb below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this alone to be safe. However, I haven't found any path to this code that doesn't involve calling the callback twice, or using internal APIs. If that's the case, the fix can be as simple as changing the error message on this line. I'll take a look in the morning.

@mcollina
Copy link
Member

@mscdex why semver-major?

@mscdex
Copy link
Contributor

mscdex commented May 19, 2017

@mcollina Change in error message?

@mcollina
Copy link
Member

@mscdex great, I think we can release it with a minor in readable-stream cc @calvinmetcalf.

I'm fine with a minor here as well, I do not think anyone is relying on this error, the whole stream is broken if the error happens anyway.

@mscdex
Copy link
Contributor

mscdex commented May 19, 2017

@mcollina I think we still have the policy that any error message changes are automatically semver-major, so I don't see that changing here. Perhaps later on down the road after the new centralized error information has been around for awhile we can revisit that policy.

@calvinmetcalf
Copy link
Contributor

yeah especially considering it wasn't a documented error message, we can get away with minor in readable-streams

@cjihrig
Copy link
Contributor Author

cjihrig commented May 19, 2017

Updated to only change the error message. CI: https://ci.nodejs.org/job/node-test-pull-request/8175/

mscdex
mscdex reviewed May 19, 2017
View reviewed changes
lib/_stream_transform.js Outdated
@@ -89,7 +89,8 @@ function afterTransform(stream, er, data) {
var cb = ts.writecb;

if (!cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add braces here since the body is now multi-line?

@cjihrig
Copy link
Contributor Author

cjihrig commented May 19, 2017

Added curly braces, and CI was green.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 19, 2017

@jasnell @abouthiroppy you may want to re-review

mcollina
mcollina approved these changes May 22, 2017
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

jasnell
jasnell approved these changes May 22, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM. We still need to get the errors in here converted over to use internal/errors tho... so that's something to keep in mind for later

@cjihrig
stream: improve multiple callback error message
f8c617d 
When a transform stream's callback is called more than once,
an error is emitted with a somewhat confusing message. This
commit hopes to improve the quality of the error message.

Fixes: nodejs#12513
PR-URL: nodejs#12520
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit f8c617d into nodejs:master May 22, 2017
@cjihrig cjihrig deleted the err-msg branch May 22, 2017 15:57
@MylesBorins
Copy link
Contributor

this technically landed after the semver major cutoff @nodejs/ctc does anyone have an issue with this landing in the 8.0.0 release?

jasnell pushed a commit that referenced this pull request May 23, 2017
@cjihrig @jasnell
stream: improve multiple callback error message
0d852a1 
When a transform stream's callback is called more than once,
an error is emitted with a somewhat confusing message. This
commit hopes to improve the quality of the error message.

Fixes: #12513
PR-URL: #12520
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request May 23, 2017
@cjihrig @jasnell
stream: improve multiple callback error message
f36c970 
When a transform stream's callback is called more than once,
an error is emitted with a somewhat confusing message. This
commit hopes to improve the quality of the error message.

Fixes: #12513
PR-URL: #12520
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kt3k kt3k kt3k left review comments

@mscdex mscdex mscdex left review comments

@hiroppy hiroppy hiroppy approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @mcollina @mscdex @calvinmetcalf @MylesBorins @jasnell @kt3k @hiroppy @nodejs-github-bot