Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

domain: Make .domain on async resources non-enumerable #26210

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
};

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
};
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

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