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

simplest domain.run use fails without nextTick if surrounded in try/catch #4570

Closed
bmeck opened this issue Jan 11, 2013 · 11 comments
Closed

Comments

@bmeck
Copy link
Member

bmeck commented Jan 11, 2013

From the REPL or other try/catch situation:

var domain = require('domain');
var d=domain.create();
d.on('error', function () {console.log('OK')})
d.run(function () {throw e})

Domain.run fails to use the domain and emit the error event unless the thrown error is on a different tick, should this be documented or changed?

@othiym23
Copy link

+1 for fixing this. Module code can throw on startup, even if the functionality is async, and those errors should be emitted on the domain.

@indutny
Copy link
Member

indutny commented Jan 11, 2013

+1 from me too. @isaacs ?

@cxreg
Copy link

cxreg commented Jan 12, 2013

I'm unable to reproduce this issue, in v0.8 it catches the error and prints 'OK'

What version are you seeing the problem on, and what is the result?

@bmeck
Copy link
Member Author

bmeck commented Jan 13, 2013

@cxreg 0.8.16

Appears to only affect the repl???

git@(unnamed branch) bradleymeck node>./node
> .load dom.js
> var domain = require('domain');
undefined
> var d=domain.create();
undefined
> d.on('error', function () {console.log('OK')})
{ domain: null,
  _events: { error: [Function] },
  _maxListeners: 10,
  members: [] }
> d.run(function () {throw e})
ReferenceError: e is not defined
    at repl:1:27
    at Domain.bind.b (domain.js:201:18)
    at Domain.run (domain.js:141:23)
    at repl:1:4
    at REPLServer.self.eval (repl.js:109:21)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)
    at Interface._ttyWrite (readline.js:783:20)
> 
(^C again to quit)
> 
git@(unnamed branch) bradleymeck node>./node -v
v0.8.16
git@(unnamed branch) bradleymeck node>./node dom.js
OK

@cxreg
Copy link

cxreg commented Jan 13, 2013

Yes, this is an issue with the repl code using try/catch, which prevents the domain's uncaughtException handler from picking up the error

This patch appears to make it work as expected: https://gist.github.com/4525700

@cxreg
Copy link

cxreg commented Jan 13, 2013

It's worth mentioning that a similar concern has come up in node_redis, and it would be nice if there was a common idiom for mixing try/catch with domains, as it places a high mental burden on developers who want to support both.

@dougwilson
Copy link
Member

It was also noted in express recently: expressjs/express#1467

@al6x
Copy link

al6x commented Jan 14, 2013

It was also noted in express recently: expressjs/express#1467

The "bug" (not actually a bug) in express is different - the problem there - inner try/catch block in expressjs, domains itself are fine.

@dougwilson
Copy link
Member

@alexeyPetrushin yes, I know, which is the same thing going on in the REPL. My comment was more for @cxreg on why there should be a common idiom for implementatiors.

@al6x
Copy link

al6x commented Jan 14, 2013

@dougwilson got it, yea, seems like concept of domain sometimes conflicting with already existing solution.

@jasnell
Copy link
Member

jasnell commented May 18, 2015

Appears to have been resolved.

@jasnell jasnell closed this as completed May 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants