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 — net: noop destroyed socket by ronag · Pull Request #30839 · nodejs/node

Conversation

@ronag

@ronag ronag commented Dec 7, 2019

Copy link
Copy Markdown
Member

_write and _read can be called from 'connect' after
Socket.destroy() has been called. This should be a noop.

Refs: #30837

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Dec 7, 2019
@ronag
ronag force-pushed the net-destroy branch 3 times, most recently from b82e63a to 0013206 Compare December 7, 2019 16:03
@ronag
net: noop destroyed socket
1efb877 
_write and _read can be called from 'connect' after
Socket.destroy() has been called. This should be a noop.

Refs: nodejs#30837
@ronag
ronag force-pushed the net-destroy branch from 0013206 to 1efb877 Compare December 7, 2019 16:03
lpinca
lpinca reviewed Dec 7, 2019
View reviewed changes
Comment thread lib/net.js
debug('_read wait for connection');
this.once('connect', () => this._read(n));
this.once('connect', () => {
if (!this.destroyed) {

@lpinca lpinca Dec 7, 2019

Copy link
Copy Markdown
Member

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.

_handle.onread is already set to a noop after destroy so I don't think this is needed. Also 'connect' should not be emitted if the socket is destroyed while connecting.

Sorry, something went wrong.

Uh oh!

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

@ronag ronag Dec 7, 2019

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.

Hm, I see. I can remove this. I still kind of like it since it makes it more explicit.

Sorry, something went wrong.

Uh oh!

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

@lpinca lpinca Dec 7, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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 would be code that is impossible to cover with tests and this is already explicit

node/lib/net.js

Lines 1106 to 1108 in cf5ce2c

if (self.destroyed) {
return;
}

Sorry, something went wrong.

Uh oh!

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

ronag commented Dec 7, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@lpinca: given your feedback this PR doesn't seem to do anything. However, we do have a different bug. The callback for write(cb) might not be called if the socket is destroyed before 'connect'.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag closed this Dec 7, 2019
@ronag ronag mentioned this pull request Dec 7, 2019
net: write(cb) not called if destroy():ed before 'connect' #30841
Closed
@lpinca

lpinca commented Dec 7, 2019

Copy link
Copy Markdown
Member

Yes, and to be honest I think it is the right thing to do as discussed in #30596. No write done no callback, but I agree behavior was changed in #29028 so it should be consistent.
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Dec 7, 2019

Copy link
Copy Markdown
Member Author

I'm fine with either as long as it's consistent and documented.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca mentioned this pull request Dec 8, 2019
stream: invoke buffered write callbacks on error #30596
Closed
4 tasks done
@lpinca lpinca mentioned this pull request Jan 5, 2020
[v13.x backport] stream: invoke buffered write callbacks on error #31179
Closed
4 tasks done
@snyk-bot snyk-bot mentioned this pull request Jun 1, 2021
[Snyk] Upgrade: gatsby, gatsby-image, gatsby-plugin-catch-links, gatsby-plugin-manifest, gatsby-plugin-no-sourcemaps, gatsby-plugin-offline, gatsby-plugin-react-helmet, gatsby-plugin-sharp, gatsby-plugin-styled-components, gatsby-remark-autolink-headers, gatsby-remark-prismjs, gatsby-source-filesystem, gatsby-transformer-remark molllyn1/node#7
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca left review comments

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.

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.

3 participants

@ronag @lpinca @nodejs-github-bot
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.