stream: fix destroy(err, cb) regression by mcollina · Pull Request #13156 · nodejs/node · GitHub

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

Does the missing semicolon pass make lint?

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

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/816/
CI: https://ci.nodejs.org/job/node-test-pull-request/8231/

LGTM

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

cc @lpinca

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

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).

Some questions.

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

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.

Cool.

"

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.

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

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).

Not feeling strongly about this... Just FYI.

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

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

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

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

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

CITGM clean

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

Landed as ccd3ead

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 <cjihrig@gmail.com> Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

mcollina requested review from calvinmetcalf, cjihrig and refack May 22, 2017 16:07

cjihrig reviewed May 22, 2017

View reviewed changes

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

cjihrig approved these changes May 22, 2017

View reviewed changes

mcollina force-pushed the double-destroy-callback branch from 502bfaf to c294477 Compare May 22, 2017 16:19

refack reviewed May 22, 2017

View reviewed changes

mcollina force-pushed the double-destroy-callback branch from c294477 to f4813b1 Compare May 22, 2017 16:48

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 added this to the 8.0.0 milestone May 22, 2017

refack reviewed May 22, 2017

View reviewed changes

stream: fix destroy(err, cb) regression

17aebdf

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

mcollina force-pushed the double-destroy-callback branch from f4813b1 to 17aebdf Compare May 22, 2017 16:53

refack approved these changes May 22, 2017

View reviewed changes

mcollina added dont-land-on-v4.x labels May 22, 2017

calvinmetcalf approved these changes May 22, 2017

View reviewed changes

refack approved these changes May 22, 2017

View reviewed changes

lpinca approved these changes May 22, 2017

View reviewed changes

mcollina mentioned this pull request May 22, 2017

stream: add destroy and _destroy methods. #12925

Closed

4 tasks

mcollina closed this May 24, 2017

mcollina deleted the double-destroy-callback branch May 24, 2017 09:22

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

leonardoadame mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#332

Open

aliscco mentioned this pull request Dec 1, 2023

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#924

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1050

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#477

Open

This was referenced May 14, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#879

Open

[Snyk] Security upgrade jest from 28.1.3 to 29.0.0 leonardoadame/node#918

Open

aliscco mentioned this pull request May 15, 2024

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#1535

Open

This was referenced May 16, 2024

[Snyk] Security upgrade jest from 27.5.1 to 29.0.0 leonardoadame/node#946

Open

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#966

Open

aliscco mentioned this pull request May 17, 2024

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#1603

Open

snyk-io Bot mentioned this pull request Oct 23, 2024

[Snyk] Fix for 5 vulnerabilities Graybar-codespace/node#93

Open

stream: fix destroy(err, cb) regression by mcollina · Pull Request #13156 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

stream: fix destroy(err, cb) regression#13156mcollina wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mcollina:double-destroy-callbackmcollina/node:double-destroy-callbackCopy head branch name to clipboard

Conversation

mcollina commented May 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 22, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 22, 2017

Uh oh!

mcollina commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented May 22, 2017

Uh oh!

refack commented May 22, 2017

Uh oh!

mcollina commented May 22, 2017

Uh oh!

mcollina commented May 22, 2017

Uh oh!

refack commented May 22, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 22, 2017

Uh oh!

mcollina commented May 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stream: fix destroy(err, cb) regression#13156
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:double-destroy-callback

mcollina May 22, 2017 •
edited
Loading