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 <apapirovski@mac.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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;
+    recvBufferSize = options.recvBufferSize;
-    this[kOptionSymbol].sendBufferSize = options.sendBufferSize;
+    sendBufferSize = options.sendBufferSize;
   }
 
   var handle = newHandle(type, lookup);
   handle.owner = this;
 
-  this._handle = handle;
+  this[async_id_symbol] = handle.getAsyncId();
-  this._receiving = false;
-  this._bindState = BIND_STATE_UNBOUND;
-  this[async_id_symbol] = this._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();
+  state.handle.recvStart();
-  socket._receiving = true;
+  state.receiving = true;
-  socket._bindState = BIND_STATE_BOUND;
+  state.bindState = BIND_STATE_BOUND;
   socket.fd = -42; // compatibility hack
 
-  if (socket[kOptionSymbol].recvBufferSize)
+  if (state.recvBufferSize)
-    bufferSize(socket, socket[kOptionSymbol].recvBufferSize, RECV_BUFFER);
+    bufferSize(socket, state.recvBufferSize, RECV_BUFFER);
 
-  if (socket[kOptionSymbol].sendBufferSize)
+  if (state.sendBufferSize)
-    bufferSize(socket, socket[kOptionSymbol].sendBufferSize, SEND_BUFFER);
+    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.lookup = oldHandle.lookup;
-  newHandle.bind = self._handle.bind;
+  newHandle.bind = oldHandle.bind;
-  newHandle.send = self._handle.send;
+  newHandle.send = oldHandle.send;
   newHandle.owner = self;
 
   // Replace the existing handle by the handle we got from master.
-  self._handle.close();
+  oldHandle.close();
-  self._handle = newHandle;
+  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) {
+  if (state.queue === undefined) {
-    self._queue = [];
+    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;
+  const state = this[kStateSymbol];
-  this._queue = undefined;
+  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) {
+  if (queue !== undefined) {
-    this._queue.push(this.close.bind(this));
+    queue.push(this.close.bind(this));
     return this;
   }
 
   healthCheck(this);
   stopReceiving(this);
-  this._handle.close();
+  state.handle.close();
-  this._handle = null;
+  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();
+  state.handle.recvStop();
-  socket._receiving = false;
+  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)
+  const handle = this[kStateSymbol].handle;
-    this._handle.ref();
+
+  if (handle)
+    handle.ref();
 
   return this;
 };
 
 
 Socket.prototype.unref = function() {
-  if (this._handle)
+  const handle = this[kStateSymbol].handle;
-    this._handle.unref();
+
+  if (handle)
+    handle.unref();
 
   return this;
 };

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) {

lib/internal/dgram.js

@@ -0,0 +1,4 @@
+'use strict';
+const kStateSymbol = Symbol('state symbol');
+
+module.exports = { kStateSymbol };

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',

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);
   }));

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')

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)));

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;
     };
 

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(() =>
+  handle.close(common.mustCall(() =>
-    strictEqual(sock4._handle.hasRef(),
+    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(() =>
+  handle.close(common.mustCall(() =>
-    strictEqual(sock6._handle.hasRef(),
+    strictEqual(handle.hasRef(),
                 false, 'udp_wrap: ipv6: not unrefed on close')));
 }
 

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(Array.isArray(socket[kStateSymbol].queue));
-    assert.strictEqual(socket._queue.length, 1);
+    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;
   }