repl: handle async error in repl #4491

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@tricknotes

I think repl should handle any error thrown.
But now async errors are unhandled in repl.

When async error thrown, process is exited with status 1.

$ node
> process.nextTick(function() { throw new Error(); });
> 
repl:1
process.nextTick(function() { throw new Error(); });
                                    ^
Error
    at repl:1:37
    at process._tickCallback (node.js:386:13)
    at process._makeCallback (node.js:304:15)

I supported async error handling.

@bnoordhuis
Node.js Foundation member

/cc @TooTallNate - you should probably review this.

@TooTallNate TooTallNate was assigned Jan 1, 2013
@TooTallNate

I actually wrote a patch similar to this in the past but @isaacs passed on it because domains should be an opt-in thing, and not activated simply when entering the REPL.

Consider a production server, which allows remote REPLs for debugging live instances. With this patch, starting a REPL would now cause a negative performance impact (if the server didn't previously use domains).

@tricknotes

Thank you for your review @TooTallNate .
I have understood that it should be depend on user whether using domains or not in REPL.

But what's really bothering me is that the process started by node -i is exited in suddenly when it catches async error.
Such as http.createServer().listen(3000) throws async error and it is well-used code.

I hope that async errors are handled in node -i.

For example, the following idea solves this issue.
REPL allows domains for opt-in thing (that is disabled in default) and the process started with node -i enable this.

How do you think about my idea?

@TooTallNate

Well the -i flag is for forcing the REPL open, so I don't think domains should be defaulted to be on there.

I would be open for an option and/or a function to enable domain-mode in a REPL instance but it should be defaulted to off always.

@bmeck

@TooTallNate I think bootstrapping node without args REPL would seem sane, any objections to the CLI REPL changing since you should never do that for production?

@Nodejs-Jenkins

Can one of the admins verify this patch?

@isaacs

Sorry, didn't see this. Did almost the same thing, but with a different test, in c0721bc

@isaacs isaacs closed this Mar 14, 2013
@AlexeyKupershtokh AlexeyKupershtokh commented on the diff Mar 15, 2013
lib/repl.js
@@ -199,6 +200,13 @@ function REPLServer(prompt, stream, eval_, useGlobal, ignoreUndefined) {
self.displayPrompt();
});
+ var replDomain = domain.create();
+ replDomain.on('error', function(e) {
+ self.outputStream.write((e.stack || e) + '\n');
+ self.bufferedCommand = '';
+ self.displayPrompt();
@AlexeyKupershtokh
AlexeyKupershtokh Mar 15, 2013

wouldn't displayPrompt(true) which preserves cursor position be better?

@isaacs
isaacs Mar 15, 2013

Yes, that would be good. Also, it should write a \n before the error stack, so that it isn't to the right of the typed line.

Patch welcome.

@TooTallNate
TooTallNate Mar 16, 2013

@AlexeyKupershtokh Actually I don't think displayPrompt(true) would matter in this case. The bufferedCommand already gets reset to ''' so the cursor ends up in the same place regardless.

@isaacs I've tried adding the preliminary \n char, but it seems that changes the behavior of normal, synchronously caught exceptions. i.e. all repl errors are now being caught by the domain. I'm not sure if this is the desired behavior or not...

@AlexeyKupershtokh
AlexeyKupershtokh Mar 17, 2013

@TooTallNate ah. But by the time async command arises we could have typed a new command. Why should repl delete it?

@isaacs
isaacs Mar 17, 2013
@tricknotes

I'm glad to hear that the feature I wanted was landed in master!
Thanks @isaacs.

@tricknotes tricknotes deleted the tricknotes:handle-async-error branch Mar 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment