From b1ae6f46ffb21f90a45d08ca5af4d36af6568e48 Mon Sep 17 00:00:00 2001 From: Jordan Harband Date: Tue, 19 Feb 2019 16:00:06 -0800 Subject: [PATCH] domain: set `.domain` non-enumerable on resources In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a `.domain` property is displayed, which is *very confusing*, especially since it has some kind of WeakReference attached to it, which is not yet a language feature. This change will prevent it from showing up in casual inspection, but will leave it available for use. --- lib/domain.js | 49 ++++++++++++++++--- test/parallel/test-domain-add-remove.js | 4 ++ .../parallel/test-domain-async-id-map-leak.js | 3 ++ test/parallel/test-domain-implicit-binding.js | 2 + test/parallel/test-domain-timer.js | 2 + 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index 02709f85c072fd..46b810857482f6 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -58,7 +58,12 @@ const asyncHook = createHook({ if (process.domain !== null && process.domain !== undefined) { // If this operation is created while in a domain, let's mark it pairing.set(asyncId, process.domain[kWeak]); - resource.domain = process.domain; + Object.defineProperty(resource, 'domain', { + configurable: true, + enumerable: false, + value: process.domain, + writable: true + }); } }, before(asyncId) { @@ -196,7 +201,12 @@ Domain.prototype._errorHandler = function(er) { var caught = false; if (!util.isPrimitive(er)) { - er.domain = this; + Object.defineProperty(er, 'domain', { + configurable: true, + enumerable: false, + value: this, + writable: true + }); er.domainThrown = true; } @@ -313,7 +323,12 @@ Domain.prototype.add = function(ee) { } } - ee.domain = this; + Object.defineProperty(ee, 'domain', { + configurable: true, + enumerable: false, + value: this, + writable: true + }); this.members.push(ee); }; @@ -352,7 +367,12 @@ function intercepted(_this, self, cb, fnargs) { var er = fnargs[0]; er.domainBound = cb; er.domainThrown = false; - er.domain = self; + Object.defineProperty(er, 'domain', { + configurable: true, + enumerable: false, + value: self, + writable: true + }); self.emit('error', er); return; } @@ -406,7 +426,12 @@ Domain.prototype.bind = function(cb) { return bound(this, self, cb, arguments); } - runBound.domain = this; + Object.defineProperty(runBound, 'domain', { + configurable: true, + enumerable: false, + value: this, + writable: true + }); return runBound; }; @@ -416,7 +441,12 @@ EventEmitter.usingDomains = true; const eventInit = EventEmitter.init; EventEmitter.init = function() { - this.domain = null; + Object.defineProperty(this, 'domain', { + configurable: true, + enumerable: false, + value: null, + writable: true + }); if (exports.active && !(this instanceof exports.Domain)) { this.domain = exports.active; } @@ -445,7 +475,12 @@ EventEmitter.prototype.emit = function(...args) { if (typeof er === 'object') { er.domainEmitter = this; - er.domain = domain; + Object.defineProperty(er, 'domain', { + configurable: true, + enumerable: false, + value: domain, + writable: true + }); er.domainThrown = false; } diff --git a/test/parallel/test-domain-add-remove.js b/test/parallel/test-domain-add-remove.js index 8e1d082125cb0b..eb6503f2b923d6 100644 --- a/test/parallel/test-domain-add-remove.js +++ b/test/parallel/test-domain-add-remove.js @@ -4,6 +4,7 @@ require('../common'); const assert = require('assert'); const domain = require('domain'); const EventEmitter = require('events'); +const isEnumerable = Function.call.bind(Object.prototype.propertyIsEnumerable); const d = new domain.Domain(); const e = new EventEmitter(); @@ -11,6 +12,7 @@ const e2 = new EventEmitter(); d.add(e); assert.strictEqual(e.domain, d); +assert.strictEqual(isEnumerable(e, 'domain'), false); // Adding the same event to a domain should not change the member count let previousMemberCount = d.members.length; @@ -19,8 +21,10 @@ assert.strictEqual(previousMemberCount, d.members.length); d.add(e2); assert.strictEqual(e2.domain, d); +assert.strictEqual(isEnumerable(e2, 'domain'), false); previousMemberCount = d.members.length; d.remove(e2); assert.notStrictEqual(e2.domain, d); +assert.strictEqual(isEnumerable(e2, 'domain'), false); assert.strictEqual(previousMemberCount - 1, d.members.length); diff --git a/test/parallel/test-domain-async-id-map-leak.js b/test/parallel/test-domain-async-id-map-leak.js index e720241841d130..8c03aa9401259a 100644 --- a/test/parallel/test-domain-async-id-map-leak.js +++ b/test/parallel/test-domain-async-id-map-leak.js @@ -6,6 +6,7 @@ const assert = require('assert'); const async_hooks = require('async_hooks'); const domain = require('domain'); const EventEmitter = require('events'); +const isEnumerable = Function.call.bind(Object.prototype.propertyIsEnumerable); // This test makes sure that the (async id → domain) map which is part of the // domain module does not get in the way of garbage collection. @@ -21,7 +22,9 @@ d.run(() => { emitter.linkToResource = resource; assert.strictEqual(emitter.domain, d); + assert.strictEqual(isEnumerable(emitter, 'domain'), false); assert.strictEqual(resource.domain, d); + assert.strictEqual(isEnumerable(resource, 'domain'), false); // This would otherwise be a circular chain now: // emitter → resource → async id ⇒ domain → emitter. diff --git a/test/parallel/test-domain-implicit-binding.js b/test/parallel/test-domain-implicit-binding.js index 13c146e4d7b332..15f6685df93b7b 100644 --- a/test/parallel/test-domain-implicit-binding.js +++ b/test/parallel/test-domain-implicit-binding.js @@ -4,6 +4,7 @@ const common = require('../common'); const assert = require('assert'); const domain = require('domain'); const fs = require('fs'); +const isEnumerable = Function.call.bind(Object.prototype.propertyIsEnumerable); { const d = new domain.Domain(); @@ -11,6 +12,7 @@ const fs = require('fs'); d.on('error', common.mustCall((err) => { assert.strictEqual(err.message, 'foobar'); assert.strictEqual(err.domain, d); + assert.strictEqual(isEnumerable(err, 'domain'), false); assert.strictEqual(err.domainEmitter, undefined); assert.strictEqual(err.domainBound, undefined); assert.strictEqual(err.domainThrown, true); diff --git a/test/parallel/test-domain-timer.js b/test/parallel/test-domain-timer.js index 67910e3d5b1934..5d288489e374a6 100644 --- a/test/parallel/test-domain-timer.js +++ b/test/parallel/test-domain-timer.js @@ -3,12 +3,14 @@ const common = require('../common'); const assert = require('assert'); const domain = require('domain'); +const isEnumerable = Function.call.bind(Object.prototype.propertyIsEnumerable); const d = new domain.Domain(); d.on('error', common.mustCall((err) => { assert.strictEqual(err.message, 'foobar'); assert.strictEqual(err.domain, d); + assert.strictEqual(isEnumerable(err, 'domain'), false); assert.strictEqual(err.domainEmitter, undefined); assert.strictEqual(err.domainBound, undefined); assert.strictEqual(err.domainThrown, true);