Skip to content

Commit

Permalink
prevent domain.intercept passing 1st arg to cb
Browse files Browse the repository at this point in the history 
GH nodejs#3379
  • Loading branch information
@wavded
wavded committed Jun 6, 2012
1 parent b9c5eee commit 61c0ee2
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 8 deletions.
9 changes: 6 additions & 3 deletions doc/api/domain.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,16 @@ with a single error handler in a single place.
var d = domain.create();

function readSomeFile(filename, cb) {
fs.readFile(filename, d.intercept(function(er, data) {
fs.readFile(filename, d.intercept(function(data) {
// note, the first argument is never passed to the
// callback since it is assumed to be the 'Error' argument
// and thus intercepted by the domain.

// if this throws, it will also be passed to the domain
// additionally, we know that 'er' will always be null,
// so the error-handling logic can be moved to the 'error'
// event on the domain instead of being repeated throughout
// the program.
return cb(er, JSON.parse(data));
return cb(null, JSON.parse(data));
}));
}

Expand Down
13 changes: 8 additions & 5 deletions lib/domain.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,16 @@ Domain.prototype.bind = function(cb, interceptError) {
var b = function() {
// disposing turns functions into no-ops
if (self._disposed) return;
var args = Array.prototype.slice.call(arguments);

This comment has been minimized.

Sign in to view

Copy link
@isaacs

isaacs Jun 6, 2012

This is extremely hazardous. I haven't benchmarked it yet, but in my experience in node, calling Array.prototype.slice.call(arguments) is an optimization killer.

Should be something like this:

var len = arguments.length;
switch (len) {
  case 0:
  case 1:
    // no args that we care about.
    args = [];
    break;
  case 2:
    // optimization for most common case: cb(er, data)
    args = [arguments[1]];
    break
  default:
    // slower for less common case: cb(er, foo, bar, baz, ...)
    var args = new Array(len - 1);
    for (var i = 1; i < len; i++) {
      args[i] = arguments[i - 1];
    }
    break;
}

This comment has been minimized.

Sign in to view

Copy link
@isaacs

isaacs Jun 6, 2012

Whoops, premature send, sorry.

Then, once you've plucked off the non-error bits, just use the first argument as a named arg, and reference it by name.

  var b = function(er, data) {

If interceptError isn't set, then dont' do any of that monkey business, and instead just do cb.apply(this, arguments), but otherwise leave the arguments object alone.

This comment has been minimized.

Sign in to view

Copy link
@wavded

wavded Jun 6, 2012

Author Owner

You are right, it is more optimized - see http://jsperf.com/arguments-slice2
I'll update the patch

if (this instanceof Domain) {
return cb.apply(this, arguments);
return cb.apply(this, args);
}

// only intercept first-arg errors if explicitly requested.
if (interceptError && arguments[0] &&
(arguments[0] instanceof Error)) {
var er = arguments[0];
if (interceptError && args[0] &&
(args[0] instanceof Error)) {
var er = args[0];
decorate(er, {
domain_bound: cb,
domain_thrown: false,
Expand All @@ -166,9 +167,11 @@ Domain.prototype.bind = function(cb, interceptError) {
self.emit('error', er);
return;
}
// remove first-arg if intercept as assumed to be the error-arg
if (interceptError && args.length) args.shift();

self.enter();
var ret = cb.apply(this, arguments);
var ret = cb.apply(this, args);
self.exit();
return ret;
};
Expand Down
7 changes: 7 additions & 0 deletions test/simple/test-domain.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ function fn(er) {
var bound = d.intercept(fn);
bound(new Error('bound'));

// intercepted should never pass first argument to callback
function fn2(data) {
assert.equal(data, 'data', 'should not be null err argument')
}

var bound = d.intercept(fn2);
bound(null, 'data');


// throwing in a bound fn is also caught,
Expand Down

0 comments on commit 61c0ee2

Please sign in to comment.