From 9b7a6914a7f0bd754e78b42b48c75851cfd6b3c4 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Thu, 8 Mar 2018 22:47:55 +0100 Subject: [PATCH] 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 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Benjamin Gruenbaum --- lib/net.js | 17 +++------- test/parallel/test-child-process-fork-net2.js | 1 + .../test-http2-misbehaving-multiplex.js | 1 + test/parallel/test-https-client-resume.js | 4 +++ test/parallel/test-net-bytes-read.js | 3 +- .../parallel/test-net-connect-options-path.js | 30 +++++++++++------- .../parallel/test-net-connect-options-port.js | 18 +++++++---- .../test-net-server-connections-child-null.js | 2 ++ .../test-net-socket-close-after-end.js | 31 +++++++++++++++++++ test/parallel/test-tls-client-resume.js | 4 +++ test/parallel/test-tls-interleave.js | 1 + test/parallel/test-tls-tlswrap-segfault.js | 1 + 12 files changed, 82 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-net-socket-close-after-end.js diff --git a/lib/net.js b/lib/net.js index 466531744eea0f..f97b68d4c4b554 100644 --- a/lib/net.js +++ b/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'); } diff --git a/test/parallel/test-child-process-fork-net2.js b/test/parallel/test-child-process-fork-net2.js index dad6b0b2d0eff0..babde351d3f05a 100644 --- a/test/parallel/test-child-process-fork-net2.js +++ b/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(); } }); diff --git a/test/parallel/test-http2-misbehaving-multiplex.js b/test/parallel/test-http2-misbehaving-multiplex.js index 4f59396e6acf3e..b0b501eacbc5dd 100644 --- a/test/parallel/test-http2-misbehaving-multiplex.js +++ b/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(); }); diff --git a/test/parallel/test-https-client-resume.js b/test/parallel/test-https-client-resume.js index 149f0ccf01ae5f..301cb63dc70310 100644 --- a/test/parallel/test-https-client-resume.js +++ b/test/parallel/test-https-client-resume.js @@ -79,5 +79,9 @@ server.listen(0, function() { console.log('close2'); server.close(); }); + + client2.resume(); }); + + client1.resume(); }); diff --git a/test/parallel/test-net-bytes-read.js b/test/parallel/test-net-bytes-read.js index aed14fe279eb5b..fa6b2383b4f317 100644 --- a/test/parallel/test-net-bytes-read.js +++ b/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(); }); diff --git a/test/parallel/test-net-connect-options-path.js b/test/parallel/test-net-connect-options-path.js index 9a2737c371bbf5..61de8caab15b56 100644 --- a/test/parallel/test-net-connect-options-path.js +++ b/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(); })); } diff --git a/test/parallel/test-net-connect-options-port.js b/test/parallel/test-net-connect-options-port.js index ea3fadf7211af6..db7a123f77b7ad 100644 --- a/test/parallel/test-net-connect-options-port.js +++ b/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(); } ]; } diff --git a/test/parallel/test-net-server-connections-child-null.js b/test/parallel/test-net-server-connections-child-null.js index cbe2d22052192d..46084404c87661 100644 --- a/test/parallel/test-net-server-connections-child-null.js +++ b/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(); })); }); diff --git a/test/parallel/test-net-socket-close-after-end.js b/test/parallel/test-net-socket-close-after-end.js new file mode 100644 index 00000000000000..06bf55f89d6e50 --- /dev/null +++ b/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'); + }); +})); diff --git a/test/parallel/test-tls-client-resume.js b/test/parallel/test-tls-client-resume.js index d99c11a3e67790..296df88e9f9152 100644 --- a/test/parallel/test-tls-client-resume.js +++ b/test/parallel/test-tls-client-resume.js @@ -73,5 +73,9 @@ server.listen(0, function() { console.log('close2'); server.close(); }); + + client2.resume(); }); + + client1.resume(); }); diff --git a/test/parallel/test-tls-interleave.js b/test/parallel/test-tls-interleave.js index c82d7140fce887..70f98f33e4f2b2 100644 --- a/test/parallel/test-tls-interleave.js +++ b/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); }); diff --git a/test/parallel/test-tls-tlswrap-segfault.js b/test/parallel/test-tls-tlswrap-segfault.js index eaa51ff51baa71..a36016efa48a02 100644 --- a/test/parallel/test-tls-tlswrap-segfault.js +++ b/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) {