net: emit 'close' after 'end'

Currently the writable side of the socket is closed as soon as `UV_EOF`
is read regardless of the state of the socket. This allows the handle
to be closed before `'end'` is emitted and thus `'close'` can be
emitted before `'end'` if the socket is paused.

This commit prevents the handle from being closed until `'end'` is
emitted ensuring the correct order of events.

PR-URL: https://github.com/nodejs/node/pull/19241
Fixes: https://github.com/nodejs/node/issues/19166
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

lib/net.js

@@ -373,8 +373,6 @@ function afterShutdown(status, handle) {
   if (self._readableState.ended) {
     debug('readableState ended, destroying');
     self.destroy();
-  } else {
-    self.once('_socketEnd', self.destroy);
   }
 }
 
@@ -530,6 +528,11 @@ Socket.prototype.end = function(data, encoding, callback) {
 
 // Called when the 'end' event is emitted.
 function onReadableStreamEnd() {
+  if (!this.allowHalfOpen) {
+    this.write = writeAfterFIN;
+    if (this.writable)
+      this.end();
+  }
   maybeDestroy(this);
 }
 
@@ -649,16 +652,6 @@ function onread(nread, buffer) {
   // `end` -> `close`
   self.push(null);
   self.read(0);
-
-  if (!self.allowHalfOpen) {
-    self.write = writeAfterFIN;
-    self.destroySoon();
-  }
-
-  // internal end event so that we know that the actual socket
-  // is no longer readable, and we can start the shutdown
-  // procedure. No need to wait for all the data to be consumed.
-  self.emit('_socketEnd');
 }
 
 

test/parallel/test-child-process-fork-net2.js

@@ -130,6 +130,7 @@ if (process.argv[2] === 'child') {
         console.error('[m] CLIENT: close event');
         disconnected += 1;
       });
+      client.resume();
     }
   });
 

test/parallel/test-http2-misbehaving-multiplex.js

@@ -66,4 +66,5 @@ server.listen(0, () => {
   // either way if it is, but we don't want to die if it is.
   client.on('error', () => {});
   client.on('close', common.mustCall(() => server.close()));
+  client.resume();
 });

test/parallel/test-https-client-resume.js

@@ -79,5 +79,9 @@ server.listen(0, function() {
       console.log('close2');
       server.close();
     });
+
+    client2.resume();
   });
+
+  client1.resume();
 });

test/parallel/test-net-bytes-read.js

@@ -32,6 +32,7 @@ const server = net.createServer((socket) => {
       assert.strictEqual(socket.bytesRead, prev);
       assert.strictEqual(big.length, prev);
     }));
+
+    socket.end();
   });
-  socket.end();
 });

test/parallel/test-net-connect-options-path.js

@@ -31,23 +31,29 @@ const CLIENT_VARIANTS = 12;
     });
 
     // CLIENT_VARIANTS depends on the following code
-    net.connect(serverPath, getConnectCb());
+    net.connect(serverPath, getConnectCb()).resume();
     net.connect(serverPath)
-      .on('connect', getConnectCb());
-    net.createConnection(serverPath, getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
+    net.createConnection(serverPath, getConnectCb()).resume();
     net.createConnection(serverPath)
-      .on('connect', getConnectCb());
-    new net.Socket().connect(serverPath, getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
+    new net.Socket().connect(serverPath, getConnectCb()).resume();
     new net.Socket().connect(serverPath)
-      .on('connect', getConnectCb());
-    net.connect({ path: serverPath }, getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
+    net.connect({ path: serverPath }, getConnectCb()).resume();
     net.connect({ path: serverPath })
-      .on('connect', getConnectCb());
-    net.createConnection({ path: serverPath }, getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
+    net.createConnection({ path: serverPath }, getConnectCb()).resume();
     net.createConnection({ path: serverPath })
-      .on('connect', getConnectCb());
-    new net.Socket().connect({ path: serverPath }, getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
+    new net.Socket().connect({ path: serverPath }, getConnectCb()).resume();
     new net.Socket().connect({ path: serverPath })
-      .on('connect', getConnectCb());
+      .on('connect', getConnectCb())
+      .resume();
   }));
 }

test/parallel/test-net-connect-options-port.js

@@ -102,27 +102,33 @@ const net = require('net');
 function doConnect(args, getCb) {
   return [
     function createConnectionWithCb() {
-      return net.createConnection.apply(net, args.concat(getCb()));
+      return net.createConnection.apply(net, args.concat(getCb()))
+        .resume();
     },
     function createConnectionWithoutCb() {
       return net.createConnection.apply(net, args)
-        .on('connect', getCb());
+        .on('connect', getCb())
+        .resume();
     },
     function connectWithCb() {
-      return net.connect.apply(net, args.concat(getCb()));
+      return net.connect.apply(net, args.concat(getCb()))
+        .resume();
     },
     function connectWithoutCb() {
       return net.connect.apply(net, args)
-        .on('connect', getCb());
+        .on('connect', getCb())
+        .resume();
     },
     function socketConnectWithCb() {
       const socket = new net.Socket();
-      return socket.connect.apply(socket, args.concat(getCb()));
+      return socket.connect.apply(socket, args.concat(getCb()))
+        .resume();
     },
     function socketConnectWithoutCb() {
       const socket = new net.Socket();
       return socket.connect.apply(socket, args)
-        .on('connect', getCb());
+        .on('connect', getCb())
+        .resume();
     }
   ];
 }

test/parallel/test-net-server-connections-child-null.js

@@ -37,6 +37,8 @@ if (process.argv[2] === 'child') {
         assert.strictEqual(server.connections, null);
         server.close();
       }));
+
+      connect.resume();
     }));
   });
 

test/parallel/test-net-socket-close-after-end.js

@@ -0,0 +1,31 @@
+'use strict';
+const common = require('../common');
+
+const assert = require('assert');
+const net = require('net');
+
+const server = net.createServer();
+
+server.on('connection', (socket) => {
+  let endEmitted = false;
+
+  socket.once('readable', () => {
+    setTimeout(() => {
+      socket.read();
+    }, common.platformTimeout(100));
+  });
+  socket.on('end', () => {
+    endEmitted = true;
+  });
+  socket.on('close', () => {
+    assert(endEmitted);
+    server.close();
+  });
+  socket.end('foo');
+});
+
+server.listen(common.mustCall(() => {
+  const socket = net.createConnection(server.address().port, () => {
+    socket.end('foo');
+  });
+}));

test/parallel/test-tls-client-resume.js

@@ -73,5 +73,9 @@ server.listen(0, function() {
       console.log('close2');
       server.close();
     });
+
+    client2.resume();
   });
+
+  client1.resume();
 });

test/parallel/test-tls-interleave.js

@@ -42,6 +42,7 @@ const writes = [
 let receivedWrites = 0;
 
 const server = tls.createServer(options, function(c) {
+  c.resume();
   writes.forEach(function(str) {
     c.write(str);
   });

test/parallel/test-tls-tlswrap-segfault.js

@@ -26,6 +26,7 @@ const server = tls.createServer(options, function(s) {
   const client = tls.connect(opts, function() {
     putImmediate(client);
   });
+  client.resume();
 });
 
 function putImmediate(client) {