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

y u no use domain? #19

Closed
carlos8f opened this issue Feb 8, 2013 · 15 comments
Closed

y u no use domain? #19

carlos8f opened this issue Feb 8, 2013 · 15 comments

Comments

@carlos8f
Copy link

carlos8f commented Feb 8, 2013

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

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

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
Copy link

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

@CrabBot
Copy link
Collaborator

CrabBot commented Feb 8, 2013

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

@carlos8f
Copy link
Author

carlos8f commented Feb 8, 2013

@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
Copy link

  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
Copy link
Author

carlos8f commented Feb 8, 2013

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

@AlexeyKupershtokh
Copy link

  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
Copy link
Author

carlos8f commented Feb 8, 2013

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
Copy link
Collaborator

CrabBot commented Feb 8, 2013

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:

  • Domains piggy-back on uncaughException prior to 0.9.x. For those keeping score, that means it's not doable in a stable branch of node.
  • As a minor annoyance, this causes the mocha tests to fail as it assumes an uncaughtException means test failure.

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

@CrabBot
Copy link
Collaborator

CrabBot commented Feb 9, 2013

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
Copy link
Collaborator

CrabBot commented Feb 9, 2013

Posted Issue: #4733

@CrabBot
Copy link
Collaborator

CrabBot commented Feb 11, 2013

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
Copy link
Author

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
Copy link
Collaborator

CrabBot commented Feb 11, 2013

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:

tlrobinson/long-stack-traces#9 (comment)
mattinsler/longjohn#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
Copy link
Owner

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.

5ca778e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants