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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

@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 •
edited
Loading

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
This was referenced Apr 28, 2018
Closed
Open
AyushG3112 added a commit to AyushG3112/node that referenced this issue May 4, 2018
@AyushG3112
doc: document that repl uses domain module for error handling
99f112d 
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
@AyushG3112 @MylesBorins
doc: document using domain in REPL
0254698 
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 <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 8, 2018
@AyushG3112 @MylesBorins
doc: document using domain in REPL
d2dd473 
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 <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 9, 2018
@AyushG3112 @MylesBorins
doc: document using domain in REPL
745e0a5 
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 <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this issue May 17, 2018
Closed
4 tasks
@BridgeAR BridgeAR mentioned this issue Apr 9, 2019
Closed
4 tasks
@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
@BridgeAR
repl: properly handle uncaughtException
cbd8eec 
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
@BridgeAR @targos
repl: handle uncaughtException properly
c78de13 
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 <lball@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Jun 3, 2020
Open
Open
@snyk-bot snyk-bot mentioned this issue Jun 14, 2020
Open
@vladnds vladnds mentioned this issue Jul 29, 2021
Closed
@kissofire kissofire mentioned this issue Nov 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
4 participants
@apapirovski @addaleax @BridgeAR @AyushG3112