repl: properly handle uncaughtException#27151

BridgeAR · 2019-04-09T13:57:18Z

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2019-04-09T14:10:38Z

Thinking about the semverness again: it should actually be semver-minor, not major. The reason is that as standalone REPL it only interacts with the user directly and the former behavior was unexpected (it was just ignored). Now it works as it would in an regular application.
If the REPL is started from inside a program and some REPL user would try to use the uncaughtException listener, it would silently have a really negative effect for the main application while not having any other effect for the person using the REPL (it would also be ignored which is a bad user experience).

BridgeAR · 2019-04-09T16:38:20Z

@nodejs/repl PTAL

nodejs-github-bot · 2019-04-09T16:39:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22313/

lance

Other than one small wording suggestion, this LGTM

doc/api/repl.md

nodejs-github-bot · 2019-04-14T12:54:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22426/

BridgeAR · 2019-04-15T16:07:31Z

@apapirovski you reviewed the older PR, would you mind taking another look?

@nodejs/repl this could use some further reviews.

BridgeAR · 2019-04-21T19:50:46Z

Landed in c709c52 🎉

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. PR-URL: nodejs#27151Fixes: nodejs#19998 Reviewed-By: Lance Ball <[email protected]>

vsemozhetbyt · 2019-04-21T20:32:57Z

Does this conflict with #25947 ? (CI is older than that landed PR).
See failing test in #27336

BridgeAR · 2019-04-21T20:38:08Z

@vsemozhetbyt yes :/ I would like to force-push it out since nothing else landed since but it's quite some time ago. @nodejs/tsc if anyone is around, are you fine with that?

addaleax · 2019-04-21T20:41:22Z

@BridgeAR If you are somewhat certain that that’s not causing a lot of trouble, I’m okay with it – it’s Sunday & a large holiday after all, and I haven’t seen anything happen that would conflict. But if you want to open a quick revert PR, here’s my 👍 to fast-tracking.

BridgeAR · 2019-04-21T20:44:35Z

@addaleax I decided to force-push since I have not seen any activity in any other PR either (besides one that I opened in the meanwhile). I'll fix the PR here and rerun the CI before landing it again.

Trott · 2019-04-30T03:48:38Z

doc/api/errors.md

+### ERR_INVALID_REPL_INPUT
+
+The input may not be used in the [`REPL`][]. All prohibited inputs are
+documented in the [`REPL`][]'s documentation.


Nit: Instead of
All prohibited inputs are documented in the [REPL][]'s documentation.
maybe this?:
The [REPL][] documentation specifies all prohibited inputs.
(Also, does it really?)

Or maybe this?:
See the [REPL][] documentation for more information.

does it really?
Two things are prohibited in the REPL: uncaughtException listeners in case the REPL is not running as standalone program and process.setUncaughtExceptionCaptureCallback(). Both of these cases are documented. I think it's not possible to use some module features but that's just a technical limitation so far.

Trott · 2019-04-30T03:55:32Z

Is there a better name for ERR_INVALID_REPL_INPUT? That makes it sound like it's about invalid input characters or something. Like, the REPL prohibits you from using emoji characters or something. But it's not really about the input; it's about trying to add a type of listener that doesn't really make sense in the REPL. I'm having troulbe coming up with good alternatives that are much shorter than ERR_UNCAUGHT_EXCEPTION_LISTENER_PROHIBITED_IN_REPL, though....

EDIT: Oh, and it only affects the interactive REPL, right? ERR_UNCAUGHT_EXCEPTION_LISTENER_PROHIBITED_IN_INTERACTIVE_REPL....

BridgeAR · 2019-05-01T11:54:09Z

@Trott

Is there a better name for ERR_INVALID_REPL_INPUT? [...] it's not really about the input; it's about trying to add a type of listener that doesn't really make sense in the REPL

I personally doubt that this would really confuse anyone since the error message would clarify what it's about and it can only happen for the user of the REPL instance, so it's a direct feedback. I also doubt that it's a problem that the error can only happen when running the REPL inside of another program (it's just not relevant for the standalone REPL).

We could go into a different direction as well and use something like ERR_PROHIBITED_API_USAGE.

test/parallel/test-repl-uncaught-exception-async.js

Trott · 2019-05-06T17:30:42Z

test/parallel/test-repl-uncaught-exception-async.js

+);
+r.close();
+
+setTimeout(()=>{


Any reason not to use process.on('exit', ...) instead of setTimeout()? That will avoid a possible race condition, or if not that, at least the appearance of a possible race condition.

I wanted to verify the timing. Using process.on('exit', ...) has no timing guarantees.

Trott · 2019-05-06T17:30:48Z

test/parallel/test-repl-uncaught-exception-async.js

+'});\n'
+);
+r.write(
+'setTimeout(() =>{\n'+


If you go with process.on('exit', ...) below, maybe this can be setImmediate()? Not essential. I just like to avoid setTimeout() in tests wherever possible.

Trott · 2019-05-06T17:42:18Z

This mostly seems good to me (aside from some minor comments that I left on one of the tests), but I'm wrestling with if we really want to special-case the programmatic use case here.

In other words, yes, adding an uncaughtException listener will mess with the "parent" program so throwing kinda makes sense. But the user can do all sorts of stuff in the programmatic REPL, like call process.exit(0) to cause the parent to exit. Is the plan to go through and try to address all those things? If that is the plan, how do we come up with a comprehensive approach? If that is not the plan, then why do it for this one thing but not other things like that?

(Sorry if this has been discussed elsewhere and I just missed it.)

Refs: #22112

Trott · 2019-05-07T15:13:56Z

I agree that it will be surprising if this reveals any problems, but just to be thorough....

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1845/ ✔️

Trott · 2019-05-07T15:15:10Z

Absurd micro-nit: Our commit guidelines say to use an imperative verb as the first word after the subsystem. So instead of "properly handle..." maybe it should be "handle ... properly"?

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 PR-URL: nodejs#27151Fixes: nodejs#19998 Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Rich Trott <[email protected]>

BridgeAR · 2019-05-08T06:18:31Z

Landed in 422e8f7 🎉

BridgeAR · 2019-05-08T06:18:55Z

(Sorry, I just realized I forgot to rerun the CI after the fixup commit)

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: #27151Fixes: #19998 Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Notable changes: * process: * Log errors using `util.inspect` in case of fatal exceptions (Ruben Bridgewater) #27243 * repl: * Add `process.on('uncaughtException')` support (Ruben Bridgewater) #27151 * stream: * Implemented `Readable.from` async iterator utility (Guy Bedford) #27660 * tls: * Expose built-in root certificates (Ben Noordhuis) #26415 * Support `net.Server` options (Luigi Pinca) #27665 * Expose `keylog` event on TLSSocket (Alba Mendez) #27654 * worker: * Added the ability to unshift messages from the `MessagePort` (Anna Henningsen) #27294

Notable changes: * esm: * Added the `--experimental-wasm-modules` flag to support WebAssembly modules (Myles Borins & Guy Bedford) #27659 * process: * Log errors using `util.inspect` in case of fatal exceptions (Ruben Bridgewater) #27243 * repl: * Add `process.on('uncaughtException')` support (Ruben Bridgewater) #27151 * stream: * Implemented `Readable.from` async iterator utility (Guy Bedford) #27660 * tls: * Expose built-in root certificates (Ben Noordhuis) #26415 * Support `net.Server` options (Luigi Pinca) #27665 * Expose `keylog` event on TLSSocket (Alba Mendez) #27654 * worker: * Added the ability to unshift messages from the `MessagePort` (Anna Henningsen) #27294 PR-URL: #27799

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 9, 2019

This comment has been minimized.
Sign in to view

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. labels Apr 9, 2019

This was referenced Apr 9, 2019
repl: emit uncaughtException #20803
Closed
Node REPL does not honor uncaughtException listeners #19998
Closed

This comment has been minimized.
Sign in to view

BridgeAR added the review wanted PRs that need reviews. label Apr 10, 2019

BridgeAR requested review from apapirovski and lance April 10, 2019 15:08

lance approved these changes Apr 11, 2019
View reviewed changes

doc/api/repl.md Outdated Show resolvedHide resolved

vsemozhetbyt reviewed Apr 11, 2019
View reviewed changes

doc/api/repl.md Outdated Show resolvedHide resolved

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2019

BridgeAR closed this Apr 21, 2019

vsemozhetbyt mentioned this pull request Apr 21, 2019
doc: add missing pull request number #27336
Closed
4 tasks

BridgeAR reopened this Apr 21, 2019

BridgeAR requested a review from mcollina April 26, 2019 16:16

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2019

Trott reviewed Apr 30, 2019
View reviewed changes

BridgeAR requested review from Trott, antsmartian, bcoe, benjamingr and jdalton May 2, 2019 21:26

Trott reviewed May 6, 2019
View reviewed changes

test/parallel/test-repl-uncaught-exception-async.jsShow resolvedHide resolved

Trott reviewed May 6, 2019
View reviewed changes

fixup! repl: properly handle uncaughtException
488d03a

Trott approved these changes May 7, 2019
View reviewed changes

BridgeAR closed this May 8, 2019

BridgeAR mentioned this pull request May 21, 2019
v12.3.0 proposal #27799
Merged
4 tasks

BethGriggs added the dont-land-on-v10.x label Aug 20, 2019

BridgeAR deleted the repl-handle-uncaught-exception branch January 20, 2020 12:03

vladnds mentioned this pull request Jul 29, 2021
TypeError [ERR_INVALID_REPL_INPUT]: Listeners for uncaughtException cannot be used in the REPL firebase/firebase-tools#3614
Closed

Uh oh!

repl: properly handle uncaughtException#27151

repl: properly handle uncaughtException #27151

Uh oh!

Conversation

BridgeAR commented Apr 9, 2019

Checklist

Uh oh!

This comment has been minimized.

This comment has been minimized.

BridgeAR commented Apr 9, 2019

Uh oh!

BridgeAR commented Apr 9, 2019

Uh oh!

nodejs-github-bot commented Apr 9, 2019

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 14, 2019

Uh oh!

BridgeAR commented Apr 15, 2019

Uh oh!

BridgeAR commented Apr 21, 2019

Uh oh!

vsemozhetbyt commented Apr 21, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 21, 2019

Uh oh!

addaleax commented Apr 21, 2019

Uh oh!

BridgeAR commented Apr 21, 2019

Uh oh!

TrottApr 30, 2019

Choose a reason for hiding this comment

Uh oh!

TrottApr 30, 2019

Choose a reason for hiding this comment

Uh oh!

BridgeARMay 1, 2019

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented May 1, 2019

Uh oh!

Uh oh!

TrottMay 6, 2019

Choose a reason for hiding this comment

Uh oh!

BridgeARMay 6, 2019

Choose a reason for hiding this comment

Uh oh!

TrottMay 6, 2019

Choose a reason for hiding this comment

Uh oh!

Trott commented May 6, 2019

Uh oh!

Trott commented May 7, 2019• edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 7, 2019

Uh oh!

BridgeAR commented May 8, 2019

Uh oh!

BridgeAR commented May 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vsemozhetbyt commented Apr 21, 2019•
edited
Loading

Trott commented Apr 30, 2019•
edited
Loading

Trott commented May 7, 2019•
edited by BridgeAR
Loading