net: use `_server` for internal book-keeping

The role of `this.server` is now split between `this._server` and
`this.server`. Where the first one is used for counting active
connections of `net.Server`, and the latter one is just a public API for
users' consumption.

The reasoning for this is simple, `TLSSocket` instances wrap
`net.Socket` instances, thus both refer to the `net.Server` through the
`this.server` property. However, only one of them should be used for
`net.Server` connection count book-keeping, otherwise double-decrement
will happen on socket destruction.

Fix: #5083
PR-URL: https://github.com/nodejs/node/pull/5262
Reviewed-By: James M Snell <jasnell@gmail.com>

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 2077bf0846314fda1f9d59fb0e8b440d51c261a2..39d91984e296ed7c99dbee9fcae0e4cf0eb79a26 100644 (file)
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -392,12 +392,6 @@ TLSSocket.prototype._init = function(socket, wrap) {
 
   this.server = options.server;
 
-  // Move the server to TLSSocket, otherwise both `socket.destroy()` and
-  // `TLSSocket.destroy()` will decrement number of connections of the TLS
-  // server, leading to misfiring `server.close()` callback
-  if (socket && socket.server === this.server)
-    socket.server = null;
-
   // For clients, we will always have either a given ca list or be using
   // default one
   const requestCert = !!options.requestCert || !options.isServer;

diff --git a/lib/net.js b/lib/net.js
index 157b197fca31c9f69e5311a9fc68c1310092358f..4ecc4e28367f9d9bf30307549f619f70fefd23d8 100644 (file)
--- a/lib/net.js
+++ b/lib/net.js
@@ -171,6 +171,10 @@ function Socket(options) {
       this.read(0);
     }
   }
+
+  // Reserve properties
+  this.server = null;
+  this._server = null;
 }
 util.inherits(Socket, stream.Duplex);
 
@@ -481,12 +485,12 @@ Socket.prototype._destroy = function(exception, cb) {
   this.destroyed = true;
   fireErrorCallbacks();
 
-  if (this.server) {
+  if (this._server) {
     COUNTER_NET_SERVER_CONNECTION_CLOSE(this);
     debug('has server');
-    this.server._connections--;
-    if (this.server._emitCloseIfDrained) {
-      this.server._emitCloseIfDrained();
+    this._server._connections--;
+    if (this._server._emitCloseIfDrained) {
+      this._server._emitCloseIfDrained();
     }
   }
 };
@@ -1416,6 +1420,7 @@ function onconnection(err, clientHandle) {
 
   self._connections++;
   socket.server = self;
+  socket._server = self;
 
   DTRACE_NET_SERVER_CONNECTION(socket);
   LTTNG_NET_SERVER_CONNECTION(socket);

diff --git a/test/parallel/test-net-stream.js b/test/parallel/test-net-stream.js
index f2b795f931bd9cc9d5a7081da5d1b4871337f255..9075d99471540a049217f695abe87fce5460152d 100644 (file)
--- a/test/parallel/test-net-stream.js
+++ b/test/parallel/test-net-stream.js
@@ -11,6 +11,7 @@ var s = new net.Stream();
 
 s.server = new net.Server();
 s.server.connections = 10;
+s._server = s.server;
 
 assert.equal(10, s.server.connections);
 s.destroy();

diff --git a/test/parallel/test-tls-server-connection-server.js b/test/parallel/test-tls-server-connection-server.js
new file mode 100644 (file)
index 0000000..534a16a
--- /dev/null
+++ b/test/parallel/test-tls-server-connection-server.js
@@ -0,0 +1,34 @@
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto) {
+  console.log('1..0 # Skipped: missing crypto');
+  return;
+}
+
+const assert = require('assert');
+const tls = require('tls');
+const fs = require('fs');
+
+const options = {
+  key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
+  cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem')
+};
+
+const server = tls.createServer(options, function(s) {
+  s.end('hello');
+}).listen(common.PORT, function() {
+  const opts = {
+    port: common.PORT,
+    rejectUnauthorized: false
+  };
+
+  server.on('connection', common.mustCall(function(socket) {
+    assert(socket.server === server);
+    server.close();
+  }));
+
+  const client = tls.connect(opts, function() {
+    client.end();
+  });
+});