GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: fix destroy(err, cb) regression by mcollina · Pull Request #13156 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented May 22, 2017

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina requested review from calvinmetcalf, cjihrig and refack May 22, 2017 16:07
cjihrig
cjihrig reviewed May 22, 2017
View reviewed changes
Comment thread test/parallel/test-stream-writable-destroy.js Outdated

@cjihrig cjihrig May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina May 22, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina force-pushed the double-destroy-callback branch 2 times, most recently from 1f399e1 to 502bfaf Compare May 22, 2017 16:11
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes May 22, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-net-socket-destroy-send.js Outdated

@cjihrig cjihrig May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina force-pushed the double-destroy-callback branch from 502bfaf to c294477 Compare May 22, 2017 16:19
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
Member Author

cc @lpinca
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack reviewed May 22, 2017
View reviewed changes
Comment thread test/parallel/test-stream-readable-destroy.js Outdated

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina force-pushed the double-destroy-callback branch from c294477 to f4813b1 Compare May 22, 2017 16:48
@mcollina mcollina added v8.x net Issues and PRs related to the net subsystem. stream Issues and PRs related to Node.js streams. and removed v8.x labels May 22, 2017
@mcollina mcollina added this to the 8.0.0 milestone May 22, 2017
refack
refack reviewed May 22, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Some questions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-net-socket-destroy-send.js Outdated

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina May 22, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Cool.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-stream-writable-destroy.js Outdated

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/streams/destroy.js Outdated

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina May 22, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@refack refack May 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented May 22, 2017

Copy link
Copy Markdown
Contributor

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 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina
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
mcollina force-pushed the double-destroy-callback branch from f4813b1 to 17aebdf Compare May 22, 2017 16:53
@refack

refack commented May 22, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
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.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes May 22, 2017
View reviewed changes
@mcollina mcollina added dont-land-on-v4.x labels May 22, 2017
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented May 22, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
calvinmetcalf
calvinmetcalf approved these changes May 22, 2017
View reviewed changes
refack
refack approved these changes May 22, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

CITGM clean

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca approved these changes May 22, 2017
View reviewed changes
@mcollina

mcollina commented May 22, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina mentioned this pull request May 22, 2017
stream: add destroy and _destroy methods. #12925
Closed
4 tasks done
@mcollina

mcollina commented May 24, 2017

Copy link
Copy Markdown
Member Author

Landed as ccd3ead
refack reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@mcollina
stream: fix destroy(err, cb) regression
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 <[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
@mcollina @jasnell
stream: fix destroy(err, cb) regression
8990726 
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
@mcollina @jasnell
stream: fix destroy(err, cb) regression
87cef63 
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
8.0.0 Release Proposal #12220
Closed
@leonardoadame leonardoadame mentioned this pull request Nov 30, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#332
Open
@aliscco 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
This was referenced Dec 1, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#371
Open
[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 leonardoadame/node#384
Open
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#427
Open
[Snyk] Security upgrade jest from 26.6.3 to 29.0.0 leonardoadame/node#438
Open
@aliscco aliscco mentioned this pull request Dec 2, 2023
[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1050
Open
@leonardoadame 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 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 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 snyk-io Bot mentioned this pull request Oct 23, 2024
[Snyk] Fix for 5 vulnerabilities Graybar-codespace/node#93
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig approved these changes
@lpinca lpinca lpinca approved these changes
+2 more reviewers
@refack refack refack approved these changes
@calvinmetcalf calvinmetcalf calvinmetcalf approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

net Issues and PRs related to the net subsystem. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mcollina @refack @calvinmetcalf @lpinca @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.