From d497ebbf9c7b5b66d355e9645b0ab0b2c61527b2 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Fri, 20 Jul 2018 23:27:40 -0400 Subject: [PATCH 1/4] dgram: hide _healthCheck() and _stopReceiving() These methods are private APIs of dgram sockets, but do not need to be exposed via the Socket prototype. PR-URL: https://github.com/nodejs/node/pull/21923 Reviewed-By: Anatoli Papirovski Reviewed-By: Wyatt Preul Reviewed-By: Matteo Collina --- lib/dgram.js | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index f3454ffc0702c4..dc664ca2c42820 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -199,7 +199,7 @@ function bufferSize(self, size, buffer) { Socket.prototype.bind = function(port_, address_ /* , callback */) { let port = port_; - this._healthCheck(); + healthCheck(this); if (this._bindState !== BIND_STATE_UNBOUND) throw new ERR_SOCKET_ALREADY_BOUND(); @@ -444,7 +444,7 @@ Socket.prototype.send = function(buffer, throw new ERR_INVALID_ARG_TYPE('address', ['string', 'falsy'], address); } - this._healthCheck(); + healthCheck(this); if (this._bindState === BIND_STATE_UNBOUND) this.bind({ port: 0, exclusive: true }, null); @@ -525,8 +525,8 @@ Socket.prototype.close = function(callback) { return this; } - this._healthCheck(); - this._stopReceiving(); + healthCheck(this); + stopReceiving(this); this._handle.close(); this._handle = null; defaultTriggerAsyncIdScope(this[async_id_symbol], @@ -544,7 +544,7 @@ function socketCloseNT(self) { Socket.prototype.address = function() { - this._healthCheck(); + healthCheck(this); var out = {}; var err = this._handle.getsockname(out); @@ -603,7 +603,7 @@ Socket.prototype.setMulticastLoopback = function(arg) { Socket.prototype.setMulticastInterface = function(interfaceAddress) { - this._healthCheck(); + healthCheck(this); if (typeof interfaceAddress !== 'string') { throw new ERR_INVALID_ARG_TYPE( @@ -618,7 +618,7 @@ Socket.prototype.setMulticastInterface = function(interfaceAddress) { Socket.prototype.addMembership = function(multicastAddress, interfaceAddress) { - this._healthCheck(); + healthCheck(this); if (!multicastAddress) { throw new ERR_MISSING_ARGS('multicastAddress'); @@ -633,7 +633,7 @@ Socket.prototype.addMembership = function(multicastAddress, Socket.prototype.dropMembership = function(multicastAddress, interfaceAddress) { - this._healthCheck(); + healthCheck(this); if (!multicastAddress) { throw new ERR_MISSING_ARGS('multicastAddress'); @@ -646,22 +646,22 @@ Socket.prototype.dropMembership = function(multicastAddress, }; -Socket.prototype._healthCheck = function() { - if (!this._handle) { +function healthCheck(socket) { + if (!socket._handle) { // Error message from dgram_legacy.js. throw new ERR_SOCKET_DGRAM_NOT_RUNNING(); } -}; +} -Socket.prototype._stopReceiving = function() { - if (!this._receiving) +function stopReceiving(socket) { + if (!socket._receiving) return; - this._handle.recvStop(); - this._receiving = false; - this.fd = null; // compatibility hack -}; + socket._handle.recvStop(); + socket._receiving = false; + socket.fd = null; // compatibility hack +} function onMessage(nread, handle, buf, rinfo) { From 79e41cc7b071228b22906b8d44d3c9c649a9628b Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 21 Jul 2018 00:56:12 -0400 Subject: [PATCH 2/4] dgram: hide underscored Socket properties dgram sockets have a fair number of exposed private properties. This commit hides them all behind a single symbol property. PR-URL: https://github.com/nodejs/node/pull/21923 Reviewed-By: Anatoli Papirovski Reviewed-By: Wyatt Preul Reviewed-By: Matteo Collina --- lib/dgram.js | 160 ++++++++++-------- lib/internal/child_process.js | 3 +- lib/internal/dgram.js | 4 + node.gyp | 1 + test/parallel/test-dgram-close-during-bind.js | 9 +- test/parallel/test-dgram-close.js | 4 +- test/parallel/test-dgram-recv-error.js | 5 +- test/parallel/test-dgram-send-error.js | 6 +- test/parallel/test-handle-wrap-isrefed.js | 30 ++-- .../test-dgram-implicit-bind-failure.js | 8 +- 10 files changed, 141 insertions(+), 89 deletions(-) create mode 100644 lib/internal/dgram.js diff --git a/lib/dgram.js b/lib/dgram.js index dc664ca2c42820..93c94c71cfaa02 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -23,6 +23,7 @@ const assert = require('assert'); const errors = require('internal/errors'); +const { kStateSymbol } = require('internal/dgram'); const { ERR_INVALID_ARG_TYPE, ERR_MISSING_ARGS, @@ -115,36 +116,40 @@ function _createSocketHandle(address, port, addressType, fd, flags) { return handle; } -const kOptionSymbol = Symbol('options symbol'); function Socket(type, listener) { EventEmitter.call(this); var lookup; + let recvBufferSize; + let sendBufferSize; - this[kOptionSymbol] = {}; if (type !== null && typeof type === 'object') { var options = type; type = options.type; lookup = options.lookup; - this[kOptionSymbol].recvBufferSize = options.recvBufferSize; - this[kOptionSymbol].sendBufferSize = options.sendBufferSize; + recvBufferSize = options.recvBufferSize; + sendBufferSize = options.sendBufferSize; } var handle = newHandle(type, lookup); handle.owner = this; - this._handle = handle; - this._receiving = false; - this._bindState = BIND_STATE_UNBOUND; - this[async_id_symbol] = this._handle.getAsyncId(); + this[async_id_symbol] = handle.getAsyncId(); this.type = type; this.fd = null; // compatibility hack - // If true - UV_UDP_REUSEADDR flag will be set - this._reuseAddr = options && options.reuseAddr; - if (typeof listener === 'function') this.on('message', listener); + + this[kStateSymbol] = { + handle, + receiving: false, + bindState: BIND_STATE_UNBOUND, + queue: undefined, + reuseAddr: options && options.reuseAddr, // Use UV_UDP_REUSEADDR if true. + recvBufferSize, + sendBufferSize + }; } util.inherits(Socket, EventEmitter); @@ -155,33 +160,37 @@ function createSocket(type, listener) { function startListening(socket) { - socket._handle.onmessage = onMessage; + const state = socket[kStateSymbol]; + + state.handle.onmessage = onMessage; // Todo: handle errors - socket._handle.recvStart(); - socket._receiving = true; - socket._bindState = BIND_STATE_BOUND; + state.handle.recvStart(); + state.receiving = true; + state.bindState = BIND_STATE_BOUND; socket.fd = -42; // compatibility hack - if (socket[kOptionSymbol].recvBufferSize) - bufferSize(socket, socket[kOptionSymbol].recvBufferSize, RECV_BUFFER); + if (state.recvBufferSize) + bufferSize(socket, state.recvBufferSize, RECV_BUFFER); - if (socket[kOptionSymbol].sendBufferSize) - bufferSize(socket, socket[kOptionSymbol].sendBufferSize, SEND_BUFFER); + if (state.sendBufferSize) + bufferSize(socket, state.sendBufferSize, SEND_BUFFER); socket.emit('listening'); } function replaceHandle(self, newHandle) { + const state = self[kStateSymbol]; + const oldHandle = state.handle; // Set up the handle that we got from master. - newHandle.lookup = self._handle.lookup; - newHandle.bind = self._handle.bind; - newHandle.send = self._handle.send; + newHandle.lookup = oldHandle.lookup; + newHandle.bind = oldHandle.bind; + newHandle.send = oldHandle.send; newHandle.owner = self; // Replace the existing handle by the handle we got from master. - self._handle.close(); - self._handle = newHandle; + oldHandle.close(); + state.handle = newHandle; } function bufferSize(self, size, buffer) { @@ -189,7 +198,7 @@ function bufferSize(self, size, buffer) { throw new ERR_SOCKET_BAD_BUFFER_SIZE(); const ctx = {}; - const ret = self._handle.bufferSize(size, buffer, ctx); + const ret = self[kStateSymbol].handle.bufferSize(size, buffer, ctx); if (ret === undefined) { throw new ERR_SOCKET_BUFFER_SIZE(ctx); } @@ -200,11 +209,12 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) { let port = port_; healthCheck(this); + const state = this[kStateSymbol]; - if (this._bindState !== BIND_STATE_UNBOUND) + if (state.bindState !== BIND_STATE_UNBOUND) throw new ERR_SOCKET_ALREADY_BOUND(); - this._bindState = BIND_STATE_BINDING; + state.bindState = BIND_STATE_BINDING; if (arguments.length && typeof arguments[arguments.length - 1] === 'function') this.once('listening', arguments[arguments.length - 1]); @@ -236,9 +246,9 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) { } // resolve address first - this._handle.lookup(address, (err, ip) => { + state.handle.lookup(address, (err, ip) => { if (err) { - this._bindState = BIND_STATE_UNBOUND; + state.bindState = BIND_STATE_UNBOUND; this.emit('error', err); return; } @@ -247,7 +257,7 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) { cluster = require('cluster'); var flags = 0; - if (this._reuseAddr) + if (state.reuseAddr) flags |= UV_UDP_REUSEADDR; if (cluster.isWorker && !exclusive) { @@ -255,11 +265,11 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) { if (err) { var ex = exceptionWithHostPort(err, 'bind', ip, port); this.emit('error', ex); - this._bindState = BIND_STATE_UNBOUND; + state.bindState = BIND_STATE_UNBOUND; return; } - if (!this._handle) + if (!state.handle) // handle has been closed in the mean time. return handle.close(); @@ -274,14 +284,14 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) { flags: flags }, onHandle); } else { - if (!this._handle) + if (!state.handle) return; // handle has been closed in the mean time - const err = this._handle.bind(ip, port || 0, flags); + const err = state.handle.bind(ip, port || 0, flags); if (err) { var ex = exceptionWithHostPort(err, 'bind', ip, port); this.emit('error', ex); - this._bindState = BIND_STATE_UNBOUND; + state.bindState = BIND_STATE_UNBOUND; // Todo: close? return; } @@ -354,14 +364,16 @@ function fixBufferList(list) { function enqueue(self, toEnqueue) { + const state = self[kStateSymbol]; + // If the send queue hasn't been initialized yet, do it, and install an // event handler that flushes the send queue after binding is done. - if (!self._queue) { - self._queue = []; + if (state.queue === undefined) { + state.queue = []; self.once('error', onListenError); self.once('listening', onListenSuccess); } - self._queue.push(toEnqueue); + state.queue.push(toEnqueue); } @@ -373,14 +385,15 @@ function onListenSuccess() { function onListenError(err) { this.removeListener('listening', onListenSuccess); - this._queue = undefined; + this[kStateSymbol].queue = undefined; this.emit('error', new ERR_SOCKET_CANNOT_SEND()); } function clearQueue() { - const queue = this._queue; - this._queue = undefined; + const state = this[kStateSymbol]; + const queue = state.queue; + state.queue = undefined; // Flush the send queue. for (var i = 0; i < queue.length; i++) @@ -446,7 +459,9 @@ Socket.prototype.send = function(buffer, healthCheck(this); - if (this._bindState === BIND_STATE_UNBOUND) + const state = this[kStateSymbol]; + + if (state.bindState === BIND_STATE_UNBOUND) this.bind({ port: 0, exclusive: true }, null); if (list.length === 0) @@ -454,7 +469,7 @@ Socket.prototype.send = function(buffer, // If the socket hasn't been bound yet, push the outbound packet onto the // send queue and send after binding is complete. - if (this._bindState !== BIND_STATE_BOUND) { + if (state.bindState !== BIND_STATE_BOUND) { enqueue(this, this.send.bind(this, list, port, address, callback)); return; } @@ -467,10 +482,12 @@ Socket.prototype.send = function(buffer, ); }; - this._handle.lookup(address, afterDns); + state.handle.lookup(address, afterDns); }; function doSend(ex, self, ip, list, address, port, callback) { + const state = self[kStateSymbol]; + if (ex) { if (typeof callback === 'function') { process.nextTick(callback, ex); @@ -479,7 +496,7 @@ function doSend(ex, self, ip, list, address, port, callback) { process.nextTick(() => self.emit('error', ex)); return; - } else if (!self._handle) { + } else if (!state.handle) { return; } @@ -492,7 +509,7 @@ function doSend(ex, self, ip, list, address, port, callback) { req.oncomplete = afterSend; } - var err = self._handle.send(req, + var err = state.handle.send(req, list, list.length, port, @@ -517,18 +534,21 @@ function afterSend(err, sent) { } Socket.prototype.close = function(callback) { + const state = this[kStateSymbol]; + const queue = state.queue; + if (typeof callback === 'function') this.on('close', callback); - if (this._queue) { - this._queue.push(this.close.bind(this)); + if (queue !== undefined) { + queue.push(this.close.bind(this)); return this; } healthCheck(this); stopReceiving(this); - this._handle.close(); - this._handle = null; + state.handle.close(); + state.handle = null; defaultTriggerAsyncIdScope(this[async_id_symbol], process.nextTick, socketCloseNT, @@ -547,7 +567,7 @@ Socket.prototype.address = function() { healthCheck(this); var out = {}; - var err = this._handle.getsockname(out); + var err = this[kStateSymbol].handle.getsockname(out); if (err) { throw errnoException(err, 'getsockname'); } @@ -557,7 +577,7 @@ Socket.prototype.address = function() { Socket.prototype.setBroadcast = function(arg) { - var err = this._handle.setBroadcast(arg ? 1 : 0); + var err = this[kStateSymbol].handle.setBroadcast(arg ? 1 : 0); if (err) { throw errnoException(err, 'setBroadcast'); } @@ -569,7 +589,7 @@ Socket.prototype.setTTL = function(ttl) { throw new ERR_INVALID_ARG_TYPE('ttl', 'number', ttl); } - var err = this._handle.setTTL(ttl); + var err = this[kStateSymbol].handle.setTTL(ttl); if (err) { throw errnoException(err, 'setTTL'); } @@ -583,7 +603,7 @@ Socket.prototype.setMulticastTTL = function(ttl) { throw new ERR_INVALID_ARG_TYPE('ttl', 'number', ttl); } - var err = this._handle.setMulticastTTL(ttl); + var err = this[kStateSymbol].handle.setMulticastTTL(ttl); if (err) { throw errnoException(err, 'setMulticastTTL'); } @@ -593,7 +613,7 @@ Socket.prototype.setMulticastTTL = function(ttl) { Socket.prototype.setMulticastLoopback = function(arg) { - var err = this._handle.setMulticastLoopback(arg ? 1 : 0); + var err = this[kStateSymbol].handle.setMulticastLoopback(arg ? 1 : 0); if (err) { throw errnoException(err, 'setMulticastLoopback'); } @@ -610,7 +630,7 @@ Socket.prototype.setMulticastInterface = function(interfaceAddress) { 'interfaceAddress', 'string', interfaceAddress); } - const err = this._handle.setMulticastInterface(interfaceAddress); + const err = this[kStateSymbol].handle.setMulticastInterface(interfaceAddress); if (err) { throw errnoException(err, 'setMulticastInterface'); } @@ -624,7 +644,8 @@ Socket.prototype.addMembership = function(multicastAddress, throw new ERR_MISSING_ARGS('multicastAddress'); } - var err = this._handle.addMembership(multicastAddress, interfaceAddress); + const { handle } = this[kStateSymbol]; + var err = handle.addMembership(multicastAddress, interfaceAddress); if (err) { throw errnoException(err, 'addMembership'); } @@ -639,7 +660,8 @@ Socket.prototype.dropMembership = function(multicastAddress, throw new ERR_MISSING_ARGS('multicastAddress'); } - var err = this._handle.dropMembership(multicastAddress, interfaceAddress); + const { handle } = this[kStateSymbol]; + var err = handle.dropMembership(multicastAddress, interfaceAddress); if (err) { throw errnoException(err, 'dropMembership'); } @@ -647,7 +669,7 @@ Socket.prototype.dropMembership = function(multicastAddress, function healthCheck(socket) { - if (!socket._handle) { + if (!socket[kStateSymbol].handle) { // Error message from dgram_legacy.js. throw new ERR_SOCKET_DGRAM_NOT_RUNNING(); } @@ -655,11 +677,13 @@ function healthCheck(socket) { function stopReceiving(socket) { - if (!socket._receiving) + const state = socket[kStateSymbol]; + + if (!state.receiving) return; - socket._handle.recvStop(); - socket._receiving = false; + state.handle.recvStop(); + state.receiving = false; socket.fd = null; // compatibility hack } @@ -675,16 +699,20 @@ function onMessage(nread, handle, buf, rinfo) { Socket.prototype.ref = function() { - if (this._handle) - this._handle.ref(); + const handle = this[kStateSymbol].handle; + + if (handle) + handle.ref(); return this; }; Socket.prototype.unref = function() { - if (this._handle) - this._handle.unref(); + const handle = this[kStateSymbol].handle; + + if (handle) + handle.unref(); return this; }; diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 1ec3f208660910..d22009505a4574 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -32,6 +32,7 @@ const { isUint8Array } = require('internal/util/types'); const spawn_sync = process.binding('spawn_sync'); const { HTTPParser } = process.binding('http_parser'); const { freeParser } = require('_http_common'); +const { kStateSymbol } = require('internal/dgram'); const { UV_EACCES, @@ -181,7 +182,7 @@ const handleConversion = { send: function(message, socket, options) { message.dgramType = socket.type; - return socket._handle; + return socket[kStateSymbol].handle; }, got: function(message, handle, emit) { diff --git a/lib/internal/dgram.js b/lib/internal/dgram.js new file mode 100644 index 00000000000000..0f333178e87378 --- /dev/null +++ b/lib/internal/dgram.js @@ -0,0 +1,4 @@ +'use strict'; +const kStateSymbol = Symbol('state symbol'); + +module.exports = { kStateSymbol }; diff --git a/node.gyp b/node.gyp index 75288114a8a4a1..0a8630cd1bff96 100644 --- a/node.gyp +++ b/node.gyp @@ -104,6 +104,7 @@ 'lib/internal/crypto/sig.js', 'lib/internal/crypto/util.js', 'lib/internal/constants.js', + 'lib/internal/dgram.js', 'lib/internal/dns/promises.js', 'lib/internal/dns/utils.js', 'lib/internal/domexception.js', diff --git a/test/parallel/test-dgram-close-during-bind.js b/test/parallel/test-dgram-close-during-bind.js index 1764f66e7338ac..4b04610e4141dc 100644 --- a/test/parallel/test-dgram-close-during-bind.js +++ b/test/parallel/test-dgram-close-during-bind.js @@ -1,13 +1,16 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const dgram = require('dgram'); +const { kStateSymbol } = require('internal/dgram'); const socket = dgram.createSocket('udp4'); -const lookup = socket._handle.lookup; +const { handle } = socket[kStateSymbol]; +const lookup = handle.lookup; // Test the scenario where the socket is closed during a bind operation. -socket._handle.bind = common.mustNotCall('bind() should not be called.'); +handle.bind = common.mustNotCall('bind() should not be called.'); -socket._handle.lookup = common.mustCall(function(address, callback) { +handle.lookup = common.mustCall(function(address, callback) { socket.close(common.mustCall(() => { lookup.call(this, address, callback); })); diff --git a/test/parallel/test-dgram-close.js b/test/parallel/test-dgram-close.js index 01aadf2aef1bef..7b49cee73bed6a 100644 --- a/test/parallel/test-dgram-close.js +++ b/test/parallel/test-dgram-close.js @@ -19,6 +19,7 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. +// Flags: --expose-internals 'use strict'; // Ensure that if a dgram socket is closed before the DNS lookup completes, it // won't crash. @@ -26,11 +27,12 @@ const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); +const { kStateSymbol } = require('internal/dgram'); const buf = Buffer.alloc(1024, 42); let socket = dgram.createSocket('udp4'); -const handle = socket._handle; +const { handle } = socket[kStateSymbol]; // get a random port for send const portGetter = dgram.createSocket('udp4') diff --git a/test/parallel/test-dgram-recv-error.js b/test/parallel/test-dgram-recv-error.js index 5470ab8f07c2a1..9850d944ce1011 100644 --- a/test/parallel/test-dgram-recv-error.js +++ b/test/parallel/test-dgram-recv-error.js @@ -1,8 +1,11 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); +const { kStateSymbol } = require('internal/dgram'); const s = dgram.createSocket('udp4'); +const { handle } = s[kStateSymbol]; s.on('error', common.mustCall((err) => { s.close(); @@ -13,4 +16,4 @@ s.on('error', common.mustCall((err) => { })); s.on('message', common.mustNotCall('no message should be received.')); -s.bind(common.mustCall(() => s._handle.onmessage(-1, s._handle, null, null))); +s.bind(common.mustCall(() => handle.onmessage(-1, handle, null, null))); diff --git a/test/parallel/test-dgram-send-error.js b/test/parallel/test-dgram-send-error.js index 73b4584cc3bc82..7757db1e2d4474 100644 --- a/test/parallel/test-dgram-send-error.js +++ b/test/parallel/test-dgram-send-error.js @@ -1,7 +1,9 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); +const { kStateSymbol } = require('internal/dgram'); const mockError = new Error('mock DNS error'); function getSocket(callback) { @@ -9,7 +11,7 @@ function getSocket(callback) { socket.on('message', common.mustNotCall('Should not receive any messages.')); socket.bind(common.mustCall(() => { - socket._handle.lookup = function(address, callback) { + socket[kStateSymbol].handle.lookup = function(address, callback) { process.nextTick(callback, mockError); }; @@ -57,7 +59,7 @@ getSocket((socket) => { ); }); - socket._handle.send = function() { + socket[kStateSymbol].handle.send = function() { return errCode; }; diff --git a/test/parallel/test-handle-wrap-isrefed.js b/test/parallel/test-handle-wrap-isrefed.js index d6d864cc350e19..cd0f5a26ca72ad 100644 --- a/test/parallel/test-handle-wrap-isrefed.js +++ b/test/parallel/test-handle-wrap-isrefed.js @@ -1,3 +1,4 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); @@ -25,22 +26,25 @@ const strictEqual = require('assert').strictEqual; const dgram = require('dgram'); +const { kStateSymbol } = require('internal/dgram'); // dgram ipv4 { const sock4 = dgram.createSocket('udp4'); - strictEqual(Object.getPrototypeOf(sock4._handle).hasOwnProperty('hasRef'), + const handle = sock4[kStateSymbol].handle; + + strictEqual(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'), true, 'udp_wrap: ipv4: hasRef() missing'); - strictEqual(sock4._handle.hasRef(), + strictEqual(handle.hasRef(), true, 'udp_wrap: ipv4: not initially refed'); sock4.unref(); - strictEqual(sock4._handle.hasRef(), + strictEqual(handle.hasRef(), false, 'udp_wrap: ipv4: unref() ineffective'); sock4.ref(); - strictEqual(sock4._handle.hasRef(), + strictEqual(handle.hasRef(), true, 'udp_wrap: ipv4: ref() ineffective'); - sock4._handle.close(common.mustCall(() => - strictEqual(sock4._handle.hasRef(), + handle.close(common.mustCall(() => + strictEqual(handle.hasRef(), false, 'udp_wrap: ipv4: not unrefed on close'))); } @@ -48,18 +52,20 @@ const dgram = require('dgram'); // dgram ipv6 { const sock6 = dgram.createSocket('udp6'); - strictEqual(Object.getPrototypeOf(sock6._handle).hasOwnProperty('hasRef'), + const handle = sock6[kStateSymbol].handle; + + strictEqual(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'), true, 'udp_wrap: ipv6: hasRef() missing'); - strictEqual(sock6._handle.hasRef(), + strictEqual(handle.hasRef(), true, 'udp_wrap: ipv6: not initially refed'); sock6.unref(); - strictEqual(sock6._handle.hasRef(), + strictEqual(handle.hasRef(), false, 'udp_wrap: ipv6: unref() ineffective'); sock6.ref(); - strictEqual(sock6._handle.hasRef(), + strictEqual(handle.hasRef(), true, 'udp_wrap: ipv6: ref() ineffective'); - sock6._handle.close(common.mustCall(() => - strictEqual(sock6._handle.hasRef(), + handle.close(common.mustCall(() => + strictEqual(handle.hasRef(), false, 'udp_wrap: ipv6: not unrefed on close'))); } diff --git a/test/sequential/test-dgram-implicit-bind-failure.js b/test/sequential/test-dgram-implicit-bind-failure.js index 2944c9aae72abe..d77db12618f3bc 100644 --- a/test/sequential/test-dgram-implicit-bind-failure.js +++ b/test/sequential/test-dgram-implicit-bind-failure.js @@ -1,8 +1,10 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); const dns = require('dns'); +const { kStateSymbol } = require('internal/dgram'); // Monkey patch dns.lookup() so that it always fails. dns.lookup = function(address, family, callback) { @@ -25,8 +27,8 @@ socket.on('error', (err) => { // should also be two listeners - this function and the dgram internal one // time error handler. dnsFailures++; - assert(Array.isArray(socket._queue)); - assert.strictEqual(socket._queue.length, 1); + assert(Array.isArray(socket[kStateSymbol].queue)); + assert.strictEqual(socket[kStateSymbol].queue.length, 1); assert.strictEqual(socket.listenerCount('error'), 2); return; } @@ -35,7 +37,7 @@ socket.on('error', (err) => { // On error, the queue should be destroyed and this function should be // the only listener. sendFailures++; - assert.strictEqual(socket._queue, undefined); + assert.strictEqual(socket[kStateSymbol].queue, undefined); assert.strictEqual(socket.listenerCount('error'), 1); return; } From 3f6ee7578103fc6f25a0711d2df98ebf39c6620d Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 21 Jul 2018 01:17:21 -0400 Subject: [PATCH 3/4] dgram: make _createSocketHandle() internal only _createSocketHandle() is used internally by the cluster module. This commit makes it internal only API. PR-URL: https://github.com/nodejs/node/pull/21923 Reviewed-By: Anatoli Papirovski Reviewed-By: Wyatt Preul Reviewed-By: Matteo Collina --- lib/dgram.js | 65 ++---------------- lib/internal/cluster/shared_handle.js | 2 +- lib/internal/dgram.js | 68 ++++++++++++++++++- .../test-dgram-create-socket-handle.js | 4 +- 4 files changed, 75 insertions(+), 64 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index 93c94c71cfaa02..a8de95111f8412 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -21,16 +21,18 @@ 'use strict'; -const assert = require('assert'); const errors = require('internal/errors'); -const { kStateSymbol } = require('internal/dgram'); +const { + kStateSymbol, + _createSocketHandle, + newHandle +} = require('internal/dgram'); const { ERR_INVALID_ARG_TYPE, ERR_MISSING_ARGS, ERR_SOCKET_ALREADY_BOUND, ERR_SOCKET_BAD_BUFFER_SIZE, ERR_SOCKET_BAD_PORT, - ERR_SOCKET_BAD_TYPE, ERR_SOCKET_BUFFER_SIZE, ERR_SOCKET_CANNOT_SEND, ERR_SOCKET_DGRAM_NOT_RUNNING @@ -47,9 +49,6 @@ const { UV_UDP_REUSEADDR } = process.binding('constants').os; const { UDP, SendWrap } = process.binding('udp_wrap'); -// Lazy load for startup performance. -let dns; - const BIND_STATE_UNBOUND = 0; const BIND_STATE_BINDING = 1; const BIND_STATE_BOUND = 2; @@ -64,59 +63,6 @@ const errnoException = errors.errnoException; const exceptionWithHostPort = errors.exceptionWithHostPort; -function lookup4(lookup, address, callback) { - return lookup(address || '127.0.0.1', 4, callback); -} - - -function lookup6(lookup, address, callback) { - return lookup(address || '::1', 6, callback); -} - - -function newHandle(type, lookup) { - if (lookup === undefined) { - if (dns === undefined) dns = require('dns'); - lookup = dns.lookup; - } else if (typeof lookup !== 'function') - throw new ERR_INVALID_ARG_TYPE('lookup', 'Function', lookup); - - if (type === 'udp4') { - const handle = new UDP(); - handle.lookup = lookup4.bind(handle, lookup); - return handle; - } - - if (type === 'udp6') { - const handle = new UDP(); - handle.lookup = lookup6.bind(handle, lookup); - handle.bind = handle.bind6; - handle.send = handle.send6; - return handle; - } - - throw new ERR_SOCKET_BAD_TYPE(); -} - - -function _createSocketHandle(address, port, addressType, fd, flags) { - // Opening an existing fd is not supported for UDP handles. - assert(typeof fd !== 'number' || fd < 0); - - var handle = newHandle(addressType); - - if (port || address) { - var err = handle.bind(address, port || 0, flags); - if (err) { - handle.close(); - return err; - } - } - - return handle; -} - - function Socket(type, listener) { EventEmitter.call(this); var lookup; @@ -739,7 +685,6 @@ Socket.prototype.getSendBufferSize = function() { module.exports = { - _createSocketHandle, createSocket, Socket }; diff --git a/lib/internal/cluster/shared_handle.js b/lib/internal/cluster/shared_handle.js index fe6042142e31f6..0bb8c44f5d630b 100644 --- a/lib/internal/cluster/shared_handle.js +++ b/lib/internal/cluster/shared_handle.js @@ -1,6 +1,6 @@ 'use strict'; const assert = require('assert'); -const dgram = require('dgram'); +const dgram = require('internal/dgram'); const net = require('net'); module.exports = SharedHandle; diff --git a/lib/internal/dgram.js b/lib/internal/dgram.js index 0f333178e87378..82b65294c21da5 100644 --- a/lib/internal/dgram.js +++ b/lib/internal/dgram.js @@ -1,4 +1,70 @@ 'use strict'; +const assert = require('assert'); +const { codes } = require('internal/errors'); +const { UDP } = process.binding('udp_wrap'); +const { ERR_INVALID_ARG_TYPE, ERR_SOCKET_BAD_TYPE } = codes; const kStateSymbol = Symbol('state symbol'); +let dns; // Lazy load for startup performance. -module.exports = { kStateSymbol }; + +function lookup4(lookup, address, callback) { + return lookup(address || '127.0.0.1', 4, callback); +} + + +function lookup6(lookup, address, callback) { + return lookup(address || '::1', 6, callback); +} + + +function newHandle(type, lookup) { + if (lookup === undefined) { + if (dns === undefined) { + dns = require('dns'); + } + + lookup = dns.lookup; + } else if (typeof lookup !== 'function') { + throw new ERR_INVALID_ARG_TYPE('lookup', 'Function', lookup); + } + + if (type === 'udp4') { + const handle = new UDP(); + + handle.lookup = lookup4.bind(handle, lookup); + return handle; + } + + if (type === 'udp6') { + const handle = new UDP(); + + handle.lookup = lookup6.bind(handle, lookup); + handle.bind = handle.bind6; + handle.send = handle.send6; + return handle; + } + + throw new ERR_SOCKET_BAD_TYPE(); +} + + +function _createSocketHandle(address, port, addressType, fd, flags) { + // Opening an existing fd is not supported for UDP handles. + assert(typeof fd !== 'number' || fd < 0); + + const handle = newHandle(addressType); + + if (port || address) { + const err = handle.bind(address, port || 0, flags); + + if (err) { + handle.close(); + return err; + } + } + + return handle; +} + + +module.exports = { kStateSymbol, _createSocketHandle, newHandle }; diff --git a/test/parallel/test-dgram-create-socket-handle.js b/test/parallel/test-dgram-create-socket-handle.js index 57480f7d2cfaa6..e49e3e8dd6c4b4 100644 --- a/test/parallel/test-dgram-create-socket-handle.js +++ b/test/parallel/test-dgram-create-socket-handle.js @@ -1,9 +1,9 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); -const dgram = require('dgram'); +const { _createSocketHandle } = require('internal/dgram'); const UDP = process.binding('udp_wrap').UDP; -const _createSocketHandle = dgram._createSocketHandle; // Throws if an "existing fd" is passed in. common.expectsError(() => { From 045b07d51833076ea0e4b2f3d7cce94baaa550a5 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 21 Jul 2018 10:29:39 -0400 Subject: [PATCH 4/4] dgram: add getters/setters for private APIs This commit makes all previously private APIs available via getters, setters, and wrapper functions. PR-URL: https://github.com/nodejs/node/pull/21923 Reviewed-By: Anatoli Papirovski Reviewed-By: Wyatt Preul Reviewed-By: Matteo Collina --- lib/dgram.js | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/lib/dgram.js b/lib/dgram.js index a8de95111f8412..292f7daf876c43 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -684,7 +684,69 @@ Socket.prototype.getSendBufferSize = function() { }; +// Legacy private APIs to be deprecated in the future. +Object.defineProperty(Socket.prototype, '_handle', { + get() { + return this[kStateSymbol].handle; + }, + set(val) { + this[kStateSymbol].handle = val; + } +}); + + +Object.defineProperty(Socket.prototype, '_receiving', { + get() { + return this[kStateSymbol].receiving; + }, + set(val) { + this[kStateSymbol].receiving = val; + } +}); + + +Object.defineProperty(Socket.prototype, '_bindState', { + get() { + return this[kStateSymbol].bindState; + }, + set(val) { + this[kStateSymbol].bindState = val; + } +}); + + +Object.defineProperty(Socket.prototype, '_queue', { + get() { + return this[kStateSymbol].queue; + }, + set(val) { + this[kStateSymbol].queue = val; + } +}); + + +Object.defineProperty(Socket.prototype, '_reuseAddr', { + get() { + return this[kStateSymbol].reuseAddr; + }, + set(val) { + this[kStateSymbol].reuseAddr = val; + } +}); + + +Socket.prototype._healthCheck = function() { + healthCheck(this); +}; + + +Socket.prototype._stopReceiving = function() { + stopReceiving(this); +}; + + module.exports = { + _createSocketHandle, createSocket, Socket };