I wonder if this if statement can be removed. readable is automatically set to false when 'end' is emitted and maybeDestroy() is called by the handler below.

Yeah, but end hasn’t necessarily been emitted here. But you’re right, the handler below was supposed to be in the else block for this.

Yes but if I'm not wrong 'end' will not be emitted on the same tick, so why aren't we only relying on the 'end' event? Is it to avoid attaching a listener?

Is it to avoid attaching a listener?

Yes, I assume that’s it. Maybe it isn’t that important?

I don't know, I don't think it will make a big difference but there is also no real advantage in removing the "fast case" so let's keep it as is :)

FWIW we are now emitting '_socketEnd' and attaching the listener all the time as 'end' is not emitted on the same tick.

I also think this branch can create a race where 'close' can be emitted before 'end' as _handle.close() is called before the 'end' event is emitted if maybeDestroy() succeeds.

TL;DR I think it's better to always rely on the 'end' event.

TL;DR I think it's better to always rely on the 'end' event.

I am cool with that if we can agree that the fast-pathing is not worth it … I like the simplicity :)

Updated!

Can we remove this event ('_socketEnd') or is it kept to avoid possible breakage?

@lpinca It’s still used in another location … I think after #18608 things could be refactored that way, yes

Gotcha.

Sorry to pester but this should be no longer needed now. readableLength is 0 so 'end' will be emitted on the next tick and it will already set readable to false.

It's also possible that the 'end' event will never be emitted, for example if onread() returns an error before reaching this point, so I wonder if it's better to only add the 'end' listener here instead of the constructor.

this should be no longer needed now

Hm, I could have sworn there were test failures when I tired that… anyway, it seems to pass now, so: Done!

It's also possible that the 'end' event will never be emitted, for example if onread() returns an error before reaching this point, so I wonder if it's better to only add the 'end' listener here instead of the constructor.

That’s not really the common case, is it? I don’t think I have a strong preference.

Indeed it isn't, I'm perfectly fine with this as is. Thanks for bearing with me.

Can you please remove the commented code?