Home Original page

net: noop destroyed socket by ronag · Pull Request #30839 · nodejs/node

@ronag

_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

@ronag 

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

Refs: nodejs#30837

lpinca

debug('_read wait for connection');
this.once('connect', () => this._read(n));
this.once('connect', () => {
if (!this.destroyed) {

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.

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.

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

if (self.destroyed) {
return;
}

@ronag

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

@ronag ronag mentioned this pull request

Dec 7, 2019

@lpinca

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

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

Open