From 9b55e470e3934adfaebe18d5b126ed6adba9ad77 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 16 Feb 2016 15:09:31 -0500 Subject: [PATCH] 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 --- lib/_tls_wrap.js | 6 ---- lib/net.js | 13 ++++--- test/parallel/test-net-stream.js | 1 + .../test-tls-server-connection-server.js | 34 +++++++++++++++++++ 4 files changed, 44 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-tls-server-connection-server.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 2077bf0846314f..39d91984e296ed 100644 --- 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 157b197fca31c9..4ecc4e28367f9d 100644 --- 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 f2b795f931bd9c..9075d99471540a 100644 --- 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 index 00000000000000..534a16a440ca6c --- /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(); + }); +});