repl: emit uncaughtException by BridgeAR · Pull Request #20803 · nodejs/node

BridgeAR · 2018-05-17T14:31:22Z

This was just a rough idea after looking into #19998.

I have no real opinion if this is a good idea or not and thought it might be good if others give their opinion. This way it seems like we would be able to actually report uncaughtException if a listener is attached.

If this is indeed something should could do, I'll also add tests.

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

addaleax · 2018-05-17T14:34:05Z

I don’t think we want the event being emitted and information being written to stderr to be mutually exclusive? I’d generally expect that the REPL shows global errors that aren’t caught, even if there is an uncaughtException listener…

Hm, to me it's similar to the "regular" error: if there's no exception listener, the app will crash. In this context it's for me printing to the repl. If the listener is attached, it should only end up there and the user has to decide what to do with it.

apapirovski · 2018-05-17T14:34:07Z

This works in the opened repl but one of the main use cases domains serve in repl is in its programmatic usage where multiple repl instances can be created. This would pretty badly break that for anyone using uncaughtException.

To me the hard part is in distinguishing which errors should go to uncaughtException and which should be written out. Everyone will have different expectations around that. I don't think just always emitting and always logging is the right answer. Not convinced it's possible to please everyone and not break anyone.

BridgeAR · 2018-05-17T14:44:41Z

Is it actually possible to detect if the repl is opened or not? In that case we could just do it there?

AyushG3112 · 2018-05-17T15:29:09Z

The question which comes to my mind is whether we should allow one REPL session a look into uncaughtExceptions of another REPL session. I'd personally be against that as it will lead to unintended situations which would be potentially confusing to users.

BridgeAR · 2018-05-24T16:11:01Z

Is there interest in trying to pursue this further? I think it would make sense in case for the opened repl in case we can distinguish that from the programmatic usage. I just do not know if we have that information / if it is possible to get that information.

addaleax · 2018-05-24T16:14:47Z

@BridgeAR We do make some customizations for the Node.js main REPL, yes – lib/internal/repl.js is the one we use for that, rather than lib/repl.js

(Would also be okay with that, yes)

BridgeAR · 2018-08-21T01:17:51Z

PTAL

I just pushed another commit that limits this functionality strictly to the very first opened repl. In case a second one is opened, it will deactivate the behavior for all.

BridgeAR · 2018-08-26T12:06:03Z

@addaleax @apapirovski @AyushG3112 what do you think about the approach?

jasnell · 2018-10-17T16:33:08Z

Ping @BridgeAR ... what's the status on this?

BridgeAR · 2018-12-02T23:34:43Z

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

PTAL. I guess this is actually getting somewhere. I am just not to sure how to write a test for this.

Trott · 2018-12-04T04:19:17Z

@nodejs/repl This could use some reviews. Any takesr? Even someone blocking it would be more useful than it languishing with no approvals and no requests for changes, IMO.

BridgeAR · 2018-12-05T21:52:30Z

Resumed build https://ci.nodejs.org/job/node-test-commit/24000/
CI https://ci.nodejs.org/job/node-test-pull-request/19477/

BridgeAR · 2018-12-13T08:16:46Z

@nodejs/repl @nodejs/tsc PTAL

targos · 2018-12-15T10:35:53Z

@BridgeAR can you post an example repl session that shows the impact of this change?

apapirovski · 2019-01-23T16:05:02Z

Yeah, that's fair. I'm having a look myself right now.

apapirovski

SGTM but is there a way to add a test?

BridgeAR · 2019-01-27T01:52:28Z

I just reworked some code here due to @apapirovski request for a test because I realized that adding an uncaughtException listener in an repl instance which is not run as standalone program is going to silence all main application errors without any chance to get any kind of notification for it.

Therefore I added some more code to fix that behavior as well: it is from now on going to throw an error if the listener is added in such a case. I could theoretically move that part into a separate PR but for me it felt right here.

While writing tests I also noticed that the former way of detecting if the REPL was run as standalone program or not was not sufficient. Currently there's no way to detect if it's the internal repl or not, so I added an internal symbol to detect that as well.

Therefore I'll ask everyone who had a look at this so far to take another look. Thanks.

BridgeAR · 2019-01-27T15:14:29Z

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

@nodejs/tsc PTAL as this is now also fixing a nasty "bug" that could be triggered when using uncaughtException in the repl (silencing the main application errors).

Trott · 2019-01-27T19:34:22Z

Since it's semver-major, should we run CITGM at this time?

addaleax · 2019-01-27T19:35:55Z

Since it's semver-major, should we run CITGM at this time?

I don’t think we have modules in CITGM that test the default REPL (aka Node’s “interactive” mode) in any way, so I’m not sure it’s worth it?

Adding an `uncaughtException` listener in an REPL instance suppresses errors in the whole application. Closing the instance won't remove those listeners either.

The internal default repl will from now on trigger `uncaughtException` handlers instead of ignoring them. The regular error output is suppressed in that case.

BridgeAR · 2019-04-07T14:39:52Z

I updated the PR and incorporated the feedback. I also added a known issue test that highlights one of the problems this PR partially fixes (using the uncaughtException listener from an REPL instance silences all errors in the application that started that instance).

PTAL

nodejs-github-bot · 2019-04-07T14:40:46Z

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

BridgeAR · 2019-04-09T13:58:31Z

Superseded by #27151

I found a way to also handle the async cases properly and this PR changed multiple times, so it felt better to open a new one instead of keeping this one around.

lance

I was a little late on the review here. But I will submit it anyway. It's been sitting unsubmitted in my browser for a couple of days.

lance · 2019-04-08T16:02:08Z

+<a id="ERR_INVALID_REPL_INPUT"></a>
+### ERR_INVALID_REPL_INPUT
+
+The input can or may not be used in the [`REPL`][]. All prohibited inputs are


I don't understand what is meant by "can or may not be used".

I thought I'll keep it broad so that it could be used for different REPL input. So far it will only be used for uncaught exception listeners, so only the may part applies. I am happy to change it to only may or what ever you have as alternative.

lance · 2019-04-08T16:03:59Z

-* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
+* Uncaught exceptions only emit the [`'uncaughtException'`][] event if the
+ `repl` is used as standalone program. If the `repl` is included anywhere in
+ another application, adding this event synchronous will throw an


What is meant by "adding this event synchronous"?

This is actually outdated in the other PR. In this version I was not able to actually handle listeners that were added asynchronously. E.g., setTimeout(() => process.on("uncaughtException", () => {})) was not detected here. This is fixed in the latest implementation.

lance · 2019-04-08T16:05:37Z

+
+```js
+process.on('uncaughtException', () => console.log('Uncaught'));
+// TypeError [ERR_INVALID_REPL_INPUT]: Unhandled exception listeners can not be


Nit: perhaps "Listeners for uncaughtException cannot be used in the REPL" is a bit clearer.

lance · 2019-04-08T16:12:29Z

 throw e;
 });

-process.on('exit', () => (Error.prepareStackTrace = origPrepareStackTrace));


This seems unrelated to the PR changes. What am I missing?

It is somewhat unrelated. In a former PR this test was impacted as well and I noticed that this is actually not necessary, so I just went ahead and removed it.

BridgeAR added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 17, 2018

BridgeAR requested a review from addaleax May 17, 2018 14:31

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 17, 2018

addaleax reviewed May 17, 2018
View reviewed changes

BridgeAR force-pushed the repl-add-uncaught-exception branch from f962429 to ca99566Compare August 21, 2018 01:16

addaleax reviewed Aug 26, 2018
View reviewed changes

Comment threadlib/repl.js Outdated
Comment threadlib/repl.js Outdated

jasnell added the stalled Issues and PRs that are stalled. label Oct 17, 2018

BridgeAR force-pushed the repl-add-uncaught-exception branch from ca99566 to 630f80fCompare December 2, 2018 23:26

BridgeAR changed the title ~~WIP repl: emit uncaughtException~~repl: emit uncaughtExceptionDec 2, 2018

BridgeAR removed wip Issues and PRs that are still a work in progress. stalled Issues and PRs that are stalled. labels Dec 2, 2018

antsmartian reviewed Dec 3, 2018
View reviewed changes

Comment threadlib/repl.js Outdated

joyeecheung mentioned this pull request Dec 5, 2018
CI failures: 20181201-20181205 nodejs/reliability#19
Closed

BridgeAR force-pushed the repl-add-uncaught-exception branch from 630f80f to f2083edCompare December 13, 2018 07:01

BridgeAR added the tsc-review label Dec 13, 2018

BridgeAR force-pushed the repl-add-uncaught-exception branch from f2083ed to 55be36bCompare January 15, 2019 15:03

apapirovski approved these changes Jan 23, 2019
View reviewed changes

Fishrock123 reviewed Jan 23, 2019
View reviewed changes

Comment threaddoc/api/repl.md Outdated

BridgeAR mentioned this pull request Jan 27, 2019
repl: minor improvements #25731
Closed
4 tasks

Trott reviewed Jan 27, 2019
View reviewed changes

Comment threaddoc/api/errors.md Outdated

Trott reviewed Jan 27, 2019
View reviewed changes

Comment threaddoc/api/repl.md Outdated

addaleax reviewed Jan 27, 2019
View reviewed changes

Comment threadlib/internal/errors.js Outdated
Comment threadlib/internal/repl.js Outdated
Comment threadlib/repl.js Outdated

addaleax mentioned this pull request Feb 18, 2019
Should Node.js standard library throw custom Error subclasses? #8342
Closed

Trott removed the tsc-review label Mar 10, 2019

BridgeAR added 2 commits April 7, 2019 15:53

test: add known issue
1aaa6b7
Adding an `uncaughtException` listener in an REPL instance suppresses errors in the whole application. Closing the instance won't remove those listeners either.

repl: emit uncaughtException
88306b2
The internal default repl will from now on trigger `uncaughtException` handlers instead of ignoring them. The regular error output is suppressed in that case.

BridgeAR force-pushed the repl-add-uncaught-exception branch from 0c848e1 to 88306b2Compare April 7, 2019 14:32

BridgeAR requested review from Fishrock123, Trott, addaleax, antsmartian, lance and richardlau April 7, 2019 14:38

richardlau reviewed Apr 7, 2019
View reviewed changes

Comment threadtest/known_issues/known_issues.status Outdated

fixup! test: add known issue
f99c8f3

BridgeAR closed this Apr 9, 2019

lance reviewed Apr 9, 2019
View reviewed changes

BridgeAR deleted the repl-add-uncaught-exception branch January 20, 2020 12:00

Uh oh!

Uh oh!

Conversation

BridgeAR commented May 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented May 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented May 17, 2018

Uh oh!

AyushG3112 commented May 17, 2018

Uh oh!

BridgeAR commented May 24, 2018

Uh oh!

addaleax commented May 24, 2018

Uh oh!

BridgeAR commented Aug 21, 2018

Uh oh!

BridgeAR commented Aug 26, 2018

Uh oh!

Uh oh!

Uh oh!

jasnell commented Oct 17, 2018

Uh oh!

BridgeAR commented Dec 2, 2018

Uh oh!

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

BridgeAR commented Dec 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 13, 2018

Uh oh!

targos commented Dec 15, 2018

Uh oh!

apapirovski commented Jan 23, 2019

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR commented Jan 27, 2019

Uh oh!

BridgeAR commented Jan 27, 2019

Uh oh!

Uh oh!

Uh oh!

Trott commented Jan 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented Jan 27, 2019

Uh oh!

BridgeAR commented Apr 7, 2019

Uh oh!

nodejs-github-bot commented Apr 7, 2019

Uh oh!

Uh oh!

BridgeAR commented Apr 9, 2019

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

BridgeAR commented May 17, 2018•
edited
Loading

apapirovski commented May 17, 2018•
edited
Loading

BridgeAR commented Dec 5, 2018•
edited
Loading