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

stream: fix destroy(err, cb) regression #13156

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM, as discovered
by @refack.

Fixes: websockets/ws#1118

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, net


write.destroy();

const expected = new Error('kaboom')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the missing semicolon pass make lint?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, I have some leftover files with linting errors under bench, and I didn't spot those :(. Updated

@mcollina mcollina force-pushed the double-destroy-callback branch 2 times, most recently from 1f399e1 to 502bfaf Compare May 22, 2017 16:11
@mcollina
Copy link
Member Author

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM


conn.on('connect', common.mustCall(function() {
conn.destroy();
conn.on('error', common.mustCall());
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth doing some assertion on the error received here.

@mcollina
Copy link
Member Author

cc @lpinca

@mcollina
Copy link
Member Author

CI (without linting errors): https://ci.nodejs.org/job/node-test-pull-request/8232/


const expected = new Error('kaboom');

// the callback will be called asynchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

If this statement is important you should assert it (set a field on expected on line 177 and assert it's there in line 175).

@mcollina mcollina added v8.x net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. and removed v8.x labels May 22, 2017
@mcollina mcollina added this to the 8.0.0 milestone May 22, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Some questions.

const assert = require('assert');

const server = net.createServer();
server.listen(0, common.mustCall(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I like it that if a function is used it's a strong indicator that you need this bound.

Copy link
Member Author

@mcollina mcollina May 22, 2017

Choose a reason for hiding this comment

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

This is just a plain old ES5 function. I tend to say the contrary: I use () => {} as a strong indicator that I need this to stay the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool.


const expected = new Error('kaboom');

// the callback will be called asynchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

"

@@ -8,7 +8,10 @@ function destroy(err, cb) {
this._writableState.destroyed;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO If the cb should stay undocumented don't put it in the signature, extract it from arguments.
My IDE (WebStorm) will intellisense it's presence.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we shouldn't use arguments ever unless we really need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

this was done in the previous PR, I would prefer to discuss that in another issue, if you want to fire a PR (I would like to get this in ASAP, keeping it only on the regression).

Copy link
Contributor

Choose a reason for hiding this comment

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

Not feeling strongly about this... Just FYI.

@refack
Copy link
Contributor

refack commented May 22, 2017

P.S. take the @ off my name in the commit comment (or remove the attribution, I don't mind), I get pinged every time you push 😄

Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
nodejs#12925.

Fixes: websockets/ws#1118
@refack
Copy link
Contributor

refack commented May 22, 2017

[question] About synchronicity of the call to cb, is there a convention?

@mcollina
Copy link
Member Author

@refack there is not. In fact, the comment was just wrong and taken from the other place I copied those examples from. That callback should be called synchronously.

@mcollina
Copy link
Member Author

@refack can you confirm this fix does not causes any more regressions, but it fixes the one I introduced?

@refack
Copy link
Contributor

refack commented May 22, 2017

@refack can you confirm this fix does not causes any more regressions, but it fixes the one I introduced?

Waiting to the CITGM to do it's dance.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

CITGM clean

@mcollina
Copy link
Member Author

@jasnell this should be on Node v8 as well, as you plan to land 330c8d7.

This might probably be short-circuited and landed if you want to avoid a couple of CITGM failures.

@mcollina
Copy link
Member Author

Landed as ccd3ead

@mcollina mcollina closed this May 24, 2017
@mcollina mcollina deleted the double-destroy-callback branch May 24, 2017 09:22
mcollina added a commit that referenced this pull request May 24, 2017
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit that referenced this pull request May 24, 2017
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
jasnell pushed a commit that referenced this pull request May 28, 2017
Fixed a regression that caused the callback passed to destroy()
to not be called if the stream was already destroyed.
This caused a regression on the ws module in CITGM introduced by
#12925.

PR-URL: #13156
Fixes: websockets/ws#1118
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants