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

net: noop destroyed socket #30839

Closed
wants to merge 1 commit into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 7, 2019

_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

@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
_write and _read can be called from 'connect' after
Socket.destroy() has been called. This should be a noop.

Refs: nodejs#30837
@@ -571,7 +572,11 @@ Socket.prototype._read = function(n) {

if (this.connecting || !this._handle) {
debug('_read wait for connection');
this.once('connect', () => this._read(n));
this.once('connect', () => {
if (!this.destroyed) {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@lpinca lpinca Dec 7, 2019

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;
}

@ronag
Copy link
Member Author

ronag commented Dec 7, 2019

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

@lpinca
Copy link
Member

lpinca commented Dec 7, 2019

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.

@ronag
Copy link
Member Author

ronag commented Dec 7, 2019

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

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants