Skip to content

Commit

Permalink
net: use _server for internal book-keeping
Browse files Browse the repository at this point in the history
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: #5262
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
indutny authored and rvagg committed Feb 18, 2016
1 parent 48fa6f6 commit a2d198c
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 10 deletions.
6 changes: 0 additions & 6 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,12 +393,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;
Expand Down
13 changes: 9 additions & 4 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@ function Socket(options) {
this.read(0);
}
}

// Reserve properties
this.server = null;
this._server = null;
}
util.inherits(Socket, stream.Duplex);

Expand Down Expand Up @@ -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();
}
}
};
Expand Down Expand Up @@ -1424,6 +1428,7 @@ function onconnection(err, clientHandle) {

self._connections++;
socket.server = self;
socket._server = self;

DTRACE_NET_SERVER_CONNECTION(socket);
LTTNG_NET_SERVER_CONNECTION(socket);
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-net-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-tls-server-connection-server.js
Original file line number Diff line number Diff line change
@@ -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();
});
});

0 comments on commit a2d198c

Please sign in to comment.