From 97dad7e33b24ce4163fdf95b8a32300f25492aef Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 27 Sep 2018 09:35:21 -0400 Subject: [PATCH 1/5] cluster: use Map to track callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a Map to avoid delete operations in callback tracking. PR-URL: https://github.com/nodejs/node/pull/23125 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Denys Otrishko --- lib/internal/cluster/utils.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/internal/cluster/utils.js b/lib/internal/cluster/utils.js index ba72ff90945d3c..94b82d039a098a 100644 --- a/lib/internal/cluster/utils.js +++ b/lib/internal/cluster/utils.js @@ -7,7 +7,7 @@ module.exports = { handles: {} // Used in tests. }; -const callbacks = {}; +const callbacks = new Map(); var seq = 0; function sendHelper(proc, message, handle, cb) { @@ -18,7 +18,7 @@ function sendHelper(proc, message, handle, cb) { message = util._extend({ cmd: 'NODE_CLUSTER' }, message); if (typeof cb === 'function') - callbacks[seq] = cb; + callbacks.set(seq, cb); message.seq = seq; seq += 1; @@ -34,9 +34,13 @@ function internal(worker, cb) { var fn = cb; - if (message.ack !== undefined && callbacks[message.ack] !== undefined) { - fn = callbacks[message.ack]; - delete callbacks[message.ack]; + if (message.ack !== undefined) { + const callback = callbacks.get(message.ack); + + if (callback !== undefined) { + fn = callback; + callbacks.delete(message.ack); + } } fn.apply(worker, arguments); From 6654c5945263a3ced718b94a89c70fdc9b061500 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 27 Sep 2018 09:49:38 -0400 Subject: [PATCH 2/5] cluster: use Map to track round robin workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/23125 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Denys Otrishko --- lib/internal/cluster/round_robin_handle.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/internal/cluster/round_robin_handle.js b/lib/internal/cluster/round_robin_handle.js index e351f433ab8c7e..9f09d9fbf76635 100644 --- a/lib/internal/cluster/round_robin_handle.js +++ b/lib/internal/cluster/round_robin_handle.js @@ -2,7 +2,6 @@ const assert = require('assert'); const net = require('net'); const { sendHelper } = require('internal/cluster/utils'); -const getOwnPropertyNames = Object.getOwnPropertyNames; const { internalBinding } = require('internal/bootstrap/loaders'); const uv = internalBinding('uv'); @@ -10,7 +9,7 @@ module.exports = RoundRobinHandle; function RoundRobinHandle(key, address, port, addressType, fd) { this.key = key; - this.all = {}; + this.all = new Map(); this.free = []; this.handles = []; this.handle = null; @@ -32,8 +31,8 @@ function RoundRobinHandle(key, address, port, addressType, fd) { } RoundRobinHandle.prototype.add = function(worker, send) { - assert(worker.id in this.all === false); - this.all[worker.id] = worker; + assert(this.all.has(worker.id) === false); + this.all.set(worker.id, worker); const done = () => { if (this.handle.getsockname) { @@ -62,16 +61,17 @@ RoundRobinHandle.prototype.add = function(worker, send) { }; RoundRobinHandle.prototype.remove = function(worker) { - if (worker.id in this.all === false) + const existed = this.all.delete(worker.id); + + if (!existed) return false; - delete this.all[worker.id]; const index = this.free.indexOf(worker); if (index !== -1) this.free.splice(index, 1); - if (getOwnPropertyNames(this.all).length !== 0) + if (this.all.size !== 0) return false; for (var handle; handle = this.handles.shift(); handle.close()) @@ -91,7 +91,7 @@ RoundRobinHandle.prototype.distribute = function(err, handle) { }; RoundRobinHandle.prototype.handoff = function(worker) { - if (worker.id in this.all === false) { + if (this.all.has(worker.id) === false) { return; // Worker is closing (or has closed) the server. } From aa899aa6d0434d4183a9a5ed86f99faf29298b11 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 27 Sep 2018 10:08:48 -0400 Subject: [PATCH 3/5] cluster: use Map to track indexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/23125 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Denys Otrishko --- lib/internal/cluster/child.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/internal/cluster/child.js b/lib/internal/cluster/child.js index 39e891214ff787..05cb6d25e81a9d 100644 --- a/lib/internal/cluster/child.js +++ b/lib/internal/cluster/child.js @@ -8,7 +8,7 @@ const Worker = require('internal/cluster/worker'); const { internal, sendHelper } = require('internal/cluster/utils'); const cluster = new EventEmitter(); const handles = {}; -const indexes = {}; +const indexes = new Map(); const noop = () => {}; module.exports = cluster; @@ -62,14 +62,18 @@ cluster._getServer = function(obj, options, cb) { options.addressType, options.fd ].join(':'); - if (indexes[indexesKey] === undefined) - indexes[indexesKey] = 0; + let index = indexes.get(indexesKey); + + if (index === undefined) + index = 0; else - indexes[indexesKey]++; + index++; + + indexes.set(indexesKey, index); const message = util._extend({ act: 'queryServer', - index: indexes[indexesKey], + index, data: null }, options); @@ -108,7 +112,7 @@ function shared(message, handle, indexesKey, cb) { handle.close = function() { send({ act: 'close', key }); delete handles[key]; - delete indexes[indexesKey]; + indexes.delete(indexesKey); return close.apply(this, arguments); }.bind(handle); assert(handles[key] === undefined); @@ -141,7 +145,7 @@ function rr(message, indexesKey, cb) { send({ act: 'close', key }); delete handles[key]; - delete indexes[indexesKey]; + indexes.delete(indexesKey); key = undefined; } From 5a50989f5bde1e7cd495922444a3ce5a4c680897 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 27 Sep 2018 10:28:52 -0400 Subject: [PATCH 4/5] cluster: use Map to track handles in cluster child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/23125 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Denys Otrishko --- lib/internal/cluster/child.js | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/internal/cluster/child.js b/lib/internal/cluster/child.js index 05cb6d25e81a9d..13a22b0186fc28 100644 --- a/lib/internal/cluster/child.js +++ b/lib/internal/cluster/child.js @@ -7,7 +7,7 @@ const { owner_symbol } = require('internal/async_hooks').symbols; const Worker = require('internal/cluster/worker'); const { internal, sendHelper } = require('internal/cluster/utils'); const cluster = new EventEmitter(); -const handles = {}; +const handles = new Map(); const indexes = new Map(); const noop = () => {}; @@ -111,12 +111,12 @@ function shared(message, handle, indexesKey, cb) { handle.close = function() { send({ act: 'close', key }); - delete handles[key]; + handles.delete(key); indexes.delete(indexesKey); return close.apply(this, arguments); }.bind(handle); - assert(handles[key] === undefined); - handles[key] = handle; + assert(handles.has(key) === false); + handles.set(key, handle); cb(message.errno, handle); } @@ -144,7 +144,7 @@ function rr(message, indexesKey, cb) { return; send({ act: 'close', key }); - delete handles[key]; + handles.delete(key); indexes.delete(indexesKey); key = undefined; } @@ -166,15 +166,15 @@ function rr(message, indexesKey, cb) { handle.getsockname = getsockname; // TCP handles only. } - assert(handles[key] === undefined); - handles[key] = handle; + assert(handles.has(key) === false); + handles.set(key, handle); cb(0, handle); } // Round-robin connection. function onconnection(message, handle) { const key = message.key; - const server = handles[key]; + const server = handles.get(key); const accepted = server !== undefined; send({ ack: message.seq, accepted }); @@ -207,17 +207,16 @@ function _disconnect(masterInitiated) { } } - for (var key in handles) { - const handle = handles[key]; - delete handles[key]; + handles.forEach((handle) => { waitingCount++; if (handle[owner_symbol]) handle[owner_symbol].close(checkWaitingCount); else handle.close(checkWaitingCount); - } + }); + handles.clear(); checkWaitingCount(); } From 847037eaeddbb4ad493a6686f61f8b2d03266ff2 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 27 Sep 2018 11:13:49 -0400 Subject: [PATCH 5/5] cluster: use Map to track handles in master MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/23125 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Denys Otrishko --- lib/internal/cluster/master.js | 33 ++++++++++++++++----------------- lib/internal/cluster/utils.js | 2 +- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js index 457b3579b12c1c..2eead0b8a3a70a 100644 --- a/lib/internal/cluster/master.js +++ b/lib/internal/cluster/master.js @@ -148,7 +148,7 @@ function removeWorker(worker) { delete cluster.workers[worker.id]; if (keys(cluster.workers).length === 0) { - assert(keys(handles).length === 0, 'Resource leak detected.'); + assert(handles.size === 0, 'Resource leak detected.'); intercom.emit('disconnect'); } } @@ -156,12 +156,10 @@ function removeWorker(worker) { function removeHandlesForWorker(worker) { assert(worker); - for (var key in handles) { - const handle = handles[key]; - + handles.forEach((handle, key) => { if (handle.remove(worker)) - delete handles[key]; - } + handles.delete(key); + }); } cluster.fork = function(env) { @@ -277,7 +275,7 @@ function queryServer(worker, message) { const key = `${message.address}:${message.port}:${message.addressType}:` + `${message.fd}:${message.index}`; - var handle = handles[key]; + var handle = handles.get(key); if (handle === undefined) { let address = message.address; @@ -302,12 +300,13 @@ function queryServer(worker, message) { constructor = SharedHandle; } - handles[key] = handle = new constructor(key, - address, - message.port, - message.addressType, - message.fd, - message.flags); + handle = new constructor(key, + address, + message.port, + message.addressType, + message.fd, + message.flags); + handles.set(key, handle); } if (!handle.data) @@ -319,11 +318,11 @@ function queryServer(worker, message) { errno: errno, key: key, ack: message.seq, - data: handles[key].data + data: handles.get(key).data }, reply); if (errno) - delete handles[key]; // Gives other workers a chance to retry. + handles.delete(key); // Gives other workers a chance to retry. send(worker, reply, handle); }); @@ -346,10 +345,10 @@ function listening(worker, message) { // removed by a prior call to removeHandlesForWorker() so guard against that. function close(worker, message) { const key = message.key; - const handle = handles[key]; + const handle = handles.get(key); if (handle && handle.remove(worker)) - delete handles[key]; + handles.delete(key); } function send(worker, message, handle, cb) { diff --git a/lib/internal/cluster/utils.js b/lib/internal/cluster/utils.js index 94b82d039a098a..c3e14cbb53a721 100644 --- a/lib/internal/cluster/utils.js +++ b/lib/internal/cluster/utils.js @@ -4,7 +4,7 @@ const util = require('util'); module.exports = { sendHelper, internal, - handles: {} // Used in tests. + handles: new Map() // Used in tests. }; const callbacks = new Map();