Sockets in Node 10.0.0 and up are not destroyed if ended before connect

[brettkiefer]

• Version: 10.0.0 and up
• Platform: Darwin CLI-4992 17.5.0 Darwin Kernel Version 17.5.0: Fri Apr 13 19:32:32 PDT 2018; root:xnu-4570.51.2~1/RELEASE_X86_64 x86_64
• Subsystem: net

Beginning with commit 9b7a691, calling .end a Node.js socket before it is connected will fail to call .destroy on the socket, so the socket will never emit a close event.

Here is a minimal repro, provided by @lpinca in dicussion on #19241 (comment):

const net = require('net');

const server = net.createServer();

server.listen(() => {
  const socket = net.createConnection(server.address().port);

  socket.on('connect', () => console.log('connect'));
  socket.on('end', () => console.log('end'));
  socket.on('close', () => console.log('close'));
  socket.end();
});

A test run illustrating the problem:

$ nvm use 9.11.1
Now using node v9.11.1 (npm v5.6.0)
$ node testSocketEnd.js
connect
end
close
^C
$ nvm use 10.0.0
Now using node v10.0.0 (npm v5.6.0)
$ node testSocketEnd.js
connect
end
^C
$ nvm use 10.4.0
Now using node v10.4.0 (npm v6.1.0)
$ node testSocketEnd.js
connect
end
^C

This occurs because when we fire end before the connect completes (a real-world example is a timeout that fires off an end on the connecting socket if it is slow) we get the ordering:

• Socket.end from client code on timeout (calls down to stream.Duplex.prototype.end, setting this.writable to false and this._writableState.ending to true)

• Socket.afterConnect(sets this.writable to true when we connect without error)

• Socket.onReadableStreamEnd (calls this.end)

• Socket.end (calls stream.Duplex.prototype.end)

• Writable.prototype.end (does NOT call endWritable because this._writableState.ending is already true from the previous call to socket.end, so this.writable stays true)

• socket.maybeDestroy (does not call socket.destroy because socket.writable is true, so this.destroy is never called and close is never emitted)

While this is not really a problem with 9b7a691, but rather an issue with afterConnect setting writable without any state checks, commit 9b7a691 removes the behavior that was masking the issue -- previous to that change we were calling destroySoon, which would call down to destroy even with .writable set to true.

This is causing a problem for at least the fairly popular IORedis (redis/ioredis#633), both in theory and in practice, and it seems as though it warrants a look, I'd be surprised if that was the only thing that was depending on those sockets being destroyed and emitting close.

I'm not sure what to propose as a good fix: I don't immediately see any internal state on the socket that looks appropriate for switching behavior in afterConnect, although I guess it peeks at the _writableState in a couple of places.

[lpinca]

I can only think of not changing the .writable value in afterConnect() if _writableState.ended is true. If someone has better ideas please share.

[brettkiefer]

FWIW, I can confirm that adding a self._writableState.ended check to afterConnect before setting self.writable, as @lpinca suggests, fixes the IORedis problem (redis/ioredis#633) in my tests without adding any unexpected behavior that I can discern. And we're already checking for _writableState.finished in destroySoon (https://github.com/nodejs/node/blob/master/lib/net.js#L566) so it seems like it's not breaking any rules to look at it.

[lpinca]

We have this issue open: #445 but I would propose a PR anyway and continue discussion there. If you have time please do that, otherwise I'll try to open it before the end of the week.

[brettkiefer]

@lpinca Is that more or less what you had in mind?

[lpinca]

Yes, that's exactly what I had in mind, thanks.