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

repl: properly handle uncaughtException #27151

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 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

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 BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 9, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot 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
@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 9, 2019

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 BridgeAR added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 9, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 9, 2019

@nodejs/repl PTAL

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the review wanted PRs that need reviews. label Apr 10, 2019
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than one small wording suggestion, this LGTM

doc/api/repl.md Outdated Show resolved Hide resolved
doc/api/repl.md Outdated Show resolved Hide resolved
@BridgeAR 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
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

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

@nodejs/repl this could use some further reviews.

@BridgeAR
Copy link
Member Author

Landed in c709c52 🎉

@BridgeAR BridgeAR closed this Apr 21, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 21, 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.

PR-URL: nodejs#27151
Fixes: nodejs#19998
Reviewed-By: Lance Ball <lball@redhat.com>
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 21, 2019

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

@BridgeAR
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

@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.

@BridgeAR BridgeAR reopened this Apr 21, 2019
@BridgeAR BridgeAR requested a review from mcollina April 26, 2019 16:16
@BridgeAR 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
### ERR_INVALID_REPL_INPUT

The input may not be used in the [`REPL`][]. All prohibited inputs are
documented in the [`REPL`][]'s documentation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe this?:

See the [REPL][] documentation for more information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Trott commented Apr 30, 2019

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
Copy link
Member Author

BridgeAR commented May 1, 2019

@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.

);
r.close();

setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Trott commented May 6, 2019

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
Copy link
Member

Trott commented May 7, 2019

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
Copy link
Member

Trott commented May 7, 2019

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"?

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 8, 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

PR-URL: nodejs#27151
Fixes: nodejs#19998
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 8, 2019

Landed in 422e8f7 🎉

@BridgeAR BridgeAR closed this May 8, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented May 8, 2019

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

targos pushed a commit that referenced this pull request 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 <lball@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request May 21, 2019
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
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
BridgeAR added a commit that referenced this pull request May 21, 2019
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 a commit that referenced this pull request May 21, 2019
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 BridgeAR deleted the repl-handle-uncaught-exception branch January 20, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node REPL does not honor uncaughtException listeners
7 participants