CrabDude / trycatch

An asynchronous domain-based exception handler with long stack traces for node.js
MIT License
246 stars 17 forks source link

y u no use domain? #19

Closed carlos8f closed 11 years ago

carlos8f commented 11 years ago
var domain = require('domain');
require('longjohn');

function trycatch (fn, cb) {
  var d = domain.create();
  d.on('error', cb);
  d.run(fn);
}
AlexeyKupershtokh commented 11 years ago

Hello. It does not work as expected with regart do trycatch nesting:

var domain = require('domain');
require('longjohn');

function trycatch (fn, cb) {
  var d = domain.create();
  d.on('error', cb);
  d.run(fn);
}

trycatch(function() {
  console.error('outer fn 1');
  trycatch(function() {
    console.error('inner fn 1');
    throw new Error('test');
    console.error('inner fn 2');
  }, function(err) {
    console.error('inner cb');
    throw err;
  });
  console.error('outer fn 2');
}, function() {
  console.error('outer cb');
});

console.error('after');

The actual output is:

wicked@wnote:~/Alawar/trycatch$ node 6.js 
outer fn 1
inner fn 1
inner cb

/home/wicked/Alawar/trycatch/node_modules/longjohn/index.js:111
      throw e;
            ^
Error: test
    at /home/wicked/Alawar/trycatch/6.js:14:11
    at Domain.bind.b (domain.js:201:18)
    at Domain.Domain.run [as run] (domain.js:141:23)
    at trycatch (/home/wicked/Alawar/trycatch/6.js:7:5)
    at /home/wicked/Alawar/trycatch/6.js:12:3
    at Domain.bind.b (domain.js:201:18)
    at Domain.Domain.run [as run] (domain.js:141:23)
    at trycatch (/home/wicked/Alawar/trycatch/6.js:7:5)
    at Object.<anonymous> (/home/wicked/Alawar/trycatch/6.js:10:1)
    at Module.Module._compile [as _compile] (module.js:449:26)
---------------------------------------------
    at trycatch (/home/wicked/Alawar/trycatch/6.js:6:5)
    at /home/wicked/Alawar/trycatch/6.js:12:3
    at Domain.bind.b (domain.js:201:18)
    at Domain.Domain.run [as run] (domain.js:141:23)
    at trycatch (/home/wicked/Alawar/trycatch/6.js:7:5)
    at Object.<anonymous> (/home/wicked/Alawar/trycatch/6.js:10:1)
    at Module.Module._compile [as _compile] (module.js:449:26)
    at Object.Module._extensions..js [as .js] (module.js:467:10)

I expect that the output would be the following:

outer fn 1
inner fn 1
inner cb
outer cb
AlexeyKupershtokh commented 11 years ago

If you could write a trycatch function body that it supported exception bubbling it would be great.

CrabBot commented 11 years ago

@carlos8f I'd prefer to use domains, but for now they're insufficient to truly implement an aysnchronous try/catch.

carlos8f commented 11 years ago

@AlexeyKupershtokh how about this?

var domain = require('domain');
require('longjohn');

function trycatch (fn, cb) {
  var d = domain.create();
  d.on('error', function (err) {
    try {
      cb(err);
    }
    catch (e) {
      if (domain.active) domain.active.exit();
      if (domain.active && !domain.active._disposed) {
        domain.active.emit('error', e);
      }
      else {
        throw e;
      }
    }
  });
  d.run(fn);
}

trycatch(function outerFn () {
  console.error('outer fn 1');
  trycatch(function innerFn () {
    console.error('inner fn 1');
    throw new Error('test');
    console.error('inner fn 2');
  }, function innerCb (err) {
    console.error('inner cb');
    throw err;
  });
  console.error('outer fn 2');
}, function outerCb () {
  console.error('outer cb');
});

console.error('after');

outputs

outer fn 1
inner fn 1
inner cb
outer cb

@CrabDude I'm curious what you mean by "truly" :) Would be something good to put in the README at least.

AlexeyKupershtokh commented 11 years ago

1) Actually I think you sould wrap cb(err) into one more domain instead of using try {} catch directly. 2) Also I've seem forgotten about the console.error('after'); in my code. But it doesn't appear in your example's output either :)

carlos8f commented 11 years ago

1) I don't think that's possible unfortunately. 2) hmm!

AlexeyKupershtokh commented 11 years ago

1) I don't think that's possible unfortunately.

Curiously. Why is that?

Also I doubt that you'll always have domain.active equal to that instance you expect it to be. What if someone has created another domain by hand and has disposed it by the time of is (domain.active) stuff.

carlos8f commented 11 years ago

Not possible since it's "by design" that if you throw inside a domain error listener, the process will crash. My workaround just dodges that crash by manually sending the error to the next outer domain.

I admit that I don't know all the ramifications going on here. Just playing devil's advocate on the project's use case. I can't find a way yet to prevent the code flow from aborting (in my example above) so that's one compelling thing about trycatch.

CrabBot commented 11 years ago

Carlos, this is all very promising. Here's what I have so far:

var domain = require('domain');

function trycatch (fn, cb) {
  var d = domain.create();
  function onError(err) {
    if (domain.active) domain.active.exit();
    if (domain.active && !domain.active._disposed) {
      try {
        domain.active.run(function() {
          cb(err);
        });
      } catch(e){
        domain.active.emit('error', e)
      }
    } else {
      cb(err);
    }
  }
  d.on('error', onError);
  try {
    d.run(fn)
  } catch(eIgnore) {}
}
module.exports = trycatch

It resolves the logs in the correct order:

outer fn 1
inner fn 1
inner cb
outer cb
outer fn 2
after

Also, there are several other reasons trycatch hasn't been switched over to domains yet either:

Because of this, I haven't looked into yet any effects domains would have on long-stack-traces.

CrabBot commented 11 years ago

Actually, this is failing on nested domains which is the primary issue with why domains don't work, but it seemed your solution may have solved it. Unfortunately, it doesn't look to be that way now that I'm playing with it. =(

Here's a failing test-case that prevents use of domains:

var domain = require('domain')
var assert = require('assert')
var count = 0

var d1 = domain.create()
d1.run(function () {
  process.nextTick(function() {
    var d2 = domain.create()
    d2.run(function () {
      process.nextTick(function() {
        // THE PROBLEM IS HERE
        // IT SEEMS domain._stack POPPED THE WRONG DOMAIN
        // WHICH RESULTS IN THE OUTER CATCH NEVER BEING CALLED
        assert.equal(domain._stack[0], d1)
        ++count
        throw new Error('test 2')
      }, 0)
    })
    d2.on('error', function(err) {
      console.log('inner')
      ;++count
      throw err;
    })
  }, 0)
})
d1.on('error', function(err) {
  console.log('outer')
  ++count
  assert.equal(err.message, 'test 2')
  assert.equal(count, 3)
  console.log('success!')
})
CrabBot commented 11 years ago

Posted Issue: #4733

CrabBot commented 11 years ago

Alright, here's an update. It looks like if we merely keep track of the parent domain, and manually run the catchFn in the parent domain, trycatch can overcome the domain nesting limitation, which I have to say is pretty sweet. You lose long-stack-traces, but long-stack-traces were always intended as a development solution, and this is more of a production solution.

Here's the domain solution I currently have that's passing all unit tests:

var domain = require('domain');

function trycatch (fn, cb) {
  var activeDomain = domain.active
    , d = domain.create()

  d.on('error', function onError(err) {
    if (!(err instanceof Error)) {
      err = new Error(err)
    }
    run(activeDomain, function() {
      cb(err)
    })
  })

  run(d, fn)
}

function run(d, fn) {
  if (d && !d._disposed) {
    try {
      d.run(fn)
    } catch(e) {
      d.emit('error', e)
    }
  } else {
    fn()
  }
}

module.exports = trycatch

Carlos, thanks for helping with the first draft.

Now I need to see if there's a way to add long-stack-traces to the domain solution, or if that still requires hooking into everything.

carlos8f commented 11 years ago

hey, looks pretty good! although, I haven't looked at the node 0.9 domain implementation yet, I wonder if the approach would be broken.

longjohn already provides long stack traces, right?

CrabBot commented 11 years ago

longjohn's using the same hack as trycatch (and long-stack-traces before it) for long-stack-traces. There's a lot of special logic surrounding how to properly wrap EventEmitter callbacks without creating memory leaks and hard to debug errors:

https://github.com/tlrobinson/long-stack-traces/issues/9#issuecomment-10214092 https://github.com/mattinsler/longjohn/issues/3

While it looks like there's a possibility longjohn has addressed some of this, I'm 100% confident the trycatch/lib/hook implementation has addressed it. I'll look into this a little more, and work out a solution.

CrabDude commented 11 years ago

First version of domain-based trycatch is up on master. Check it out and let me know what you think. It's actually kind of a Franken-solution. Long-stack-traces are off by default, which means no low-level shimming / hooking is required, which is really nice.

If you decide to turn them on, you should call trycatch.configure({'long-stack-traces':true}) at the beginning of your code to allow trycatch to shim the core modules as early as possible.

https://github.com/CrabDude/trycatch/commit/5ca778eb93040ac998539c2c2d0bbde9c4c93647