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

Node REPL does not honor uncaughtException listeners #19998

Closed
AyushG3112 opened this issue Apr 13, 2018 · 7 comments
Closed

Node REPL does not honor uncaughtException listeners #19998

AyushG3112 opened this issue Apr 13, 2018 · 7 comments
Labels
doc Issues and PRs related to the documentations. domain Issues and PRs related to the domain subsystem. repl Issues and PRs related to the REPL subsystem.

Comments

@AyushG3112
Copy link
Contributor

REPL commands

$ node
> process.on('uncaughtException', (err) => console.log('caught' + err));
> throw 'hi';

OUTPUT :

Thrown: hi

Running process.hasUncaughtExceptionCaptureCallback() in the REPL console outputs true, so it is understandable why the listener isn't called.

However

> process.setUncaughtExceptionCaptureCallback(null);

throws the Error:

Error [ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE]: The `domain` module is in use, which is mutually exclusive with calling process.setUncaughtExceptionCaptureCallback()
    at process.setUncaughtExceptionCaptureCallback (domain.js:97:15)
    at repl:1:9
    at Script.runInThisContext (vm.js:91:20)
    at REPLServer.defaultEval (repl.js:311:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
    at REPLServer.onLine (repl.js:609:10)
    at REPLServer.emit (events.js:187:15)
    at REPLServer.emit (domain.js:442:20)
    at REPLServer.Interface._onLine (readline.js:285:10)
----------------------------------------
Error: require(`domain`) at this point
    at domain.js:93:28
    at NativeModule.compile (internal/bootstrap/loaders.js:209:7)
    at NativeModule.require (internal/bootstrap/loaders.js:137:18)
    at repl.js:63:16
    at NativeModule.compile (internal/bootstrap/loaders.js:209:7)
    at NativeModule.require (internal/bootstrap/loaders.js:137:18)
    at internal/repl.js:4:14
    at NativeModule.compile (internal/bootstrap/loaders.js:209:7)
    at Function.NativeModule.require (internal/bootstrap/loaders.js:137:18)
    at startup (internal/bootstrap/node.js:240:40)

I could not find any documentation of this behaviour for the Node REPL.

Is this a bug, or is this a desired behaviour which has to be documented?

@AyushG3112 AyushG3112 changed the title REPL does not honor uncaughtException listeners Node REPL does not honor uncaughtException listeners Apr 13, 2018
@addaleax
Copy link
Member

This is happening because the repl uses the domain module to do its own error handling, so I don’t really think it’s a bug.

We could maybe be a bit more forward and explicitly disable process.on('uncaughtException') in the REPL?

@addaleax addaleax added domain Issues and PRs related to the domain subsystem. repl Issues and PRs related to the REPL subsystem. labels Apr 13, 2018
@AyushG3112
Copy link
Contributor Author

AyushG3112 commented Apr 13, 2018

@addaleax That's what I thought to, but instead of disabling process.on('uncaughtException'), can't we remove the default error handling from domain module if any uncaughtException listeners exist, with a message to the user?

In either case, we do need to document this.

@AyushG3112
Copy link
Contributor Author

AyushG3112 commented Apr 13, 2018

I'll open a PR this Monday to document this behaviour, unless someone beats me to it or we decide to change something.

@AyushG3112
Copy link
Contributor Author

@addaleax before I start on documenting this, 1 question: what're your thoughts on replacing the error handling via domain with a default process.on('uncaughtException') listener?

@addaleax
Copy link
Member

@AyushG3112 My thoughts are that that’s probably very hard to get that to work, because in general there can be multiple REPL instances in a process, which need to keep track of async context in order to determine whether they should handle an exception

@apapirovski apapirovski added the doc Issues and PRs related to the documentations. label Apr 18, 2018
AyushG3112 added a commit to AyushG3112/node that referenced this issue May 4, 2018
Document that REPL uses the `domain` module to handle uncaught
exceptions, and the side effects caused by it.

Fixes: nodejs#19998
MylesBorins pushed a commit that referenced this issue May 8, 2018
Document that REPL uses the `domain` module to handle uncaught
exceptions, and the side effects caused by it.

PR-URL: #20382
Fixes: #19998
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 8, 2018
Document that REPL uses the `domain` module to handle uncaught
exceptions, and the side effects caused by it.

PR-URL: #20382
Fixes: #19998
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 9, 2018
Document that REPL uses the `domain` module to handle uncaught
exceptions, and the side effects caused by it.

PR-URL: #20382
Fixes: #19998
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2019

Reopened, since I have a fix for this. See #27151

@BridgeAR BridgeAR reopened this Apr 9, 2019
BridgeAR added a commit to BridgeAR/node that referenced this issue Apr 25, 2019
When running the REPL as standalone program it's now possible to use
`process.on('uncaughtException', listener)`. It is going to use those
listeners from now on and the regular error output is suppressed.

It also fixes the issue that REPL instances started inside of an
application would silence all application errors. It is now prohibited
to add the exception listener in such REPL instances. Trying to add
such listeners throws an `ERR_INVALID_REPL_INPUT` error.

Fixes: nodejs#19998
@BridgeAR
Copy link
Member

BridgeAR commented May 2, 2019

Reopened due to a revert due to test failures.

@BridgeAR BridgeAR reopened this May 2, 2019
targos pushed a commit that referenced this issue May 9, 2019
When running the REPL as standalone program it's now possible to use
`process.on('uncaughtException', listener)`. It is going to use those
listeners from now on and the regular error output is suppressed.

It also fixes the issue that REPL instances started inside of an
application would silence all application errors. It is now prohibited
to add the exception listener in such REPL instances. Trying to add
such listeners throws an `ERR_INVALID_REPL_INPUT` error.

Fixes: #19998

PR-URL: #27151
Fixes: #19998
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. domain Issues and PRs related to the domain subsystem. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
4 participants