Skip to content

Commit

Permalink
dgram: hide underscored Socket properties
Browse files Browse the repository at this point in the history
dgram sockets have a fair number of exposed private properties.
This commit hides them all behind a single symbol property.

PR-URL: #21923
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
cjihrig authored and targos committed Jul 31, 2018
1 parent b5b7438 commit ae17d18
Show file tree
Hide file tree
Showing 10 changed files with 141 additions and 89 deletions.
160 changes: 94 additions & 66 deletions lib/dgram.js

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/dgram.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const kStateSymbol = Symbol('state symbol');

module.exports = { kStateSymbol };
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
9 changes: 6 additions & 3 deletions test/parallel/test-dgram-close-during-bind.js
Original file line number Diff line number Diff line change
@@ -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);
}));
Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-dgram-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@
// 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.

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')
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-dgram-recv-error.js
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -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)));
6 changes: 4 additions & 2 deletions test/parallel/test-dgram-send-error.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
// 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) {
const socket = dgram.createSocket('udp4');

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

Expand Down Expand Up @@ -57,7 +59,7 @@ getSocket((socket) => {
);
});

socket._handle.send = function() {
socket[kStateSymbol].handle.send = function() {
return errCode;
};

Expand Down
30 changes: 18 additions & 12 deletions test/parallel/test-handle-wrap-isrefed.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Flags: --expose-internals
'use strict';

const common = require('../common');
Expand Down Expand Up @@ -25,41 +26,46 @@ 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')));
}


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

Expand Down
8 changes: 5 additions & 3 deletions test/sequential/test-dgram-implicit-bind-failure.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down

0 comments on commit ae17d18

Please sign in to comment.