Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Nested async callbacks and "domain" error handling #3908

Closed
lancejpollard opened this issue Aug 23, 2012 · 7 comments
Closed

Nested async callbacks and "domain" error handling #3908

lancejpollard opened this issue Aug 23, 2012 · 7 comments
Labels

Comments

@lancejpollard
Copy link

Say you have some ORM accessing a database asynchronously. If you bind to the callback, the node require('domain') module works fine:

var domain = require('domain');

var globalDomain = domain.create();

App.Post.all(globalDomain.bind(function() {
  throw new Error('an error');
}));

The issue is, this doesn't work using run, because run calls the bind function, and bind expects it to be async (unless it's a core EventEmitter like process, so run works on process.nextTick, not sure how this is working...). So in the following, the error is crashes node because it's not caught:

globalDomain.run(function() {
  App.Post.all(function() {
    throw new Error('an error');
  });
});

Because run doesn't work here, you might think to just wrap all functions in globalDomain.bind, such as this (which does work):

App.Post.all(globalDomain.bind(function() {
  App.Post.all(globalDomain.bind(function() {
    throw new Error('an error');
  })); 
}));

The main issue is that the Domain#bind function expects the callback to be synchronous. Here is where the callback passed to bind is called:

self.enter();
var ret = cb.apply(this, arguments);
self.exit();

That Domain#exit function sets the "active domain" (exports.active) to stack[stack.length - 1], which can be empty from what it looks like:

Domain.prototype.exit = function() {
  if (this._disposed) return;

  // exit all domains until this one.
  var d;
  do {
    d = stack.pop();
  } while (d && d !== this);

  exports.active = stack[stack.length - 1];
  process.domain = exports.active;
};

If there's only 1 item in the stack, it will pop it, so exports.active is undefined. This makes it so you can't get use this domain functionality.

Sorry for the rambling, just trying to figure out if this can work. Just realized the error may be b/c of that var d = stack.pop(), but initially thought it may be that there should be some callback functionality implemented in the bind method, such as:

self.enter();
cb(function() {
  self.exit();
  return ret;
});

Then the bind or run API would require a done callback:

globalDomain.bind(function(done) {
  App.Post.all(function() {
    App.Post.all(function() {
      throw new Error('an error');
      // if it never gets here that's fine, uncaughtException will handle it
      done();
    });
  });
});

Although looking at it now maybe that would be an easy way to create a garbage collection problem. Hmm... What are your thoughts about this?

@wavded
Copy link

wavded commented Sep 11, 2012

Believe I'm running into the same issue. From an API standpoint I would expect domain.run to catch stuff for any of its children, but i have to explicitly bind to every async function in order for exceptions to actually be caught, and did try that initially but had some serious memory leak issues (which may be my own lack of understanding how these things work)

@isaacs
Copy link

isaacs commented Sep 17, 2012

Unfortunately, there is no way to get at functions at creation-time. Even if we could, consider this:

d1.enter()
doAsync(theCallback)
d1.exit()
d2.enter()
doAsync(theCallback)
d2.exit()

// which is equivalent to:
d1.run(function() {
  doAsync(theCallback)
})
d2.run(function() {
  doAsync(theCallback)
})

If theCallback throws, how do we know which domain it should be in?

@wavded
Copy link

wavded commented Sep 18, 2012

@isaacs I'm confused why this works then?

var domain = require('domain')                                       
var d = domain.create()

function doAsync(cb) {
  setTimeout(cb, 200);
}

function theCallback() {
  make.exception // intentionally access property on undefined object
}

d.run(function () {
  doAsync(theCallback)                                                                                   
})

d.on('error', function (er) {
  console.log('caught it', er) // exception is caught
})

output:

caught it { [ReferenceError: make is not defined]                                           
  domain_thrown: true,                                                                      
  domain: { members: [], _events: { error: [Function] } } }

@rlidwka
Copy link

rlidwka commented Sep 25, 2012

It's because setTimeout implicitly binds async functions by itself.

To all: by the way, how should 3rd party libraries work with domains? It'll be nice to have some guidelines about it. Maybe that ORM could have implicit bindings too...

@CrabDude
Copy link

This looks like a bug, but maybe I'm not fully understanding the limitations of domains.

@isaacs Your example passes Errors to the appropriate domains. Am I missing something?
@rlidwka This is true of all internal async functionality FWICT. (e.g., fs.open, EventEmitter, etc...)

I tried @viatropos' code with mongoose and it did in fact fail to catch, even though multiple nestings of asynchronicity with http.createServer, setTimeout, and fs.readFile all succeeded. This leads me to wonder why it is failing, esp. since (FWICT) all core async callbacks are called with an active domain if the parent was created with an active domain. This should be the case in @viatropos' example; should it not?

Successful nested example w/multiple domains:

var fs = require('fs')
var domain = require('domain')
var d1 = domain.create()
var d2 = domain.create()

function onError(er) {
  console.log('caught it from: ', er.domain.name)
  console.log(er.stack) // exception is caught
}

d1.name = '1'
d2.name = '2'
d1.on('error', onError)
d2.on('error', onError)

d1.run(function() {
  setTimeout(function() {
    fs.readFile('dne', function() {
      throw new Error('1')
    })
  }, 10)
})

d2.run(function() {
  setTimeout(function() {
    fs.readFile('dne', function() {
      throw new Error('2')
    })
  }, 10)
})

Unsuccessful example using mongoose, similar to the OP's:

var mongoose = require('mongoose')
var db = mongoose.createConnection('localhost', 'test')
var schema = mongoose.Schema({ name: 'string' })
var Cat = db.model('Cat', schema)

var domain = require('domain')
var d1 = domain.create()

function doAsync(cb) {
  var kitty = new Cat({ name: 'Zildjian' })
  kitty.save(cb)
}

function theCallback() {
  throw new Error('meow!')
}

function onError(er) {
  console.log('caught it')
  console.log(er.stack) // exception is caught
}

d1.on('error', onError)

d1.run(function() {
  doAsync(theCallback)
})

WRT whether this is possible, trycatch handles this situation just fine:

var mongoose = require('mongoose')
var db = mongoose.createConnection('localhost', 'test')
var schema = mongoose.Schema({ name: 'string' })
var Cat = db.model('Cat', schema)

var trycatch = require('trycatch')

function doAsync(cb) {
  var kitty = new Cat({ name: 'Zildjian' })
  kitty.save(cb)
}

function theCallback() {
  throw new Error('meow!')
}

function onError(er) {
  console.log('caught it')
  console.log(er.stack) // exception is caught
}

trycatch(function() {
  doAsync(theCallback)
}, onError)

So the high-level goal is certainly possible. What do you mean then by "no way to get at functions at creation-time", and how does that relate to the original issue of an ORM's callbacks, etc...?

FWIW, I want to get rid of the V8 stack-trace hack in trycatch and replace it with domains, if possible, so my criticism is out of a desire to address real needs in production: stability (uncaught exceptions, memory, unstable 3rd-party modules), long-stack traces, error coalescing and 500 responses.

@fresheneesz
Copy link

I can't quite tell if this is related to my issue: http://stackoverflow.com/questions/19628611/can-domains-be-nested-in-node-js

I basically want a way for a nested domain to say "I don't know how to handle this, give it to the parent domain". I can't figure out how I would do that without explicitly knowing the parent domain.

@jasnell
Copy link
Member

jasnell commented Jun 24, 2015

@joyent/node-tsc ... any reason to keep this one open?

@Trott Trott closed this as completed Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants