Skip to content

Commit

Permalink
domain: set .domain non-enumerable on resources
Browse files Browse the repository at this point in the history
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.

PR-URL: #26210
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
  • Loading branch information
ljharb authored and BridgeAR committed Mar 14, 2019
1 parent 6f68570 commit 56adebf
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 8 deletions.
56 changes: 48 additions & 8 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,23 @@ 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
});
if (resource.promise !== undefined &&
resource.promise instanceof Promise) {
// resource.promise instanceof Promise make sure that the
// promise comes from the same context
// see https://github.com/nodejs/node/issues/15673
resource.promise.domain = process.domain;
Object.defineProperty(resource.promise, 'domain', {
configurable: true,
enumerable: false,
value: process.domain,
writable: true
});
}
}
},
Expand Down Expand Up @@ -203,7 +213,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;
}

Expand Down Expand Up @@ -320,7 +335,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);
};

Expand Down Expand Up @@ -359,7 +379,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;
}
Expand Down Expand Up @@ -413,7 +438,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;
};
Expand All @@ -423,7 +453,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;
}
Expand Down Expand Up @@ -452,7 +487,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;
}

Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-domain-add-remove.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ 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();
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;
Expand All @@ -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);
3 changes: 3 additions & 0 deletions test/parallel/test-domain-async-id-map-leak.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-domain-implicit-binding.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ 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();

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);
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-domain-timer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 56adebf

Please sign in to comment.