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

repl: do not swallow errors in nested REPLs #23004

Closed
wants to merge 2 commits into from
Closed

repl: do not swallow errors in nested REPLs #23004

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 21, 2018

For tab completion, a REPLServer instance will sometimes create another
REPLServer instance. If a callback is sent to the .complete() function
and that callback throws an error, it will be swallowed by the nested
REPLs domain. Re-throw the error so that processes don't silently exit
without any indication of an error (including a status code).

Fixes: #21586

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@Trott
repl: do not swallow errors in nested REPLs
5903929 
For tab completion, a REPLServer instance will sometimes create another
REPLServer instance. If a callback is sent to the `.complete()` function
and that callback throws an error, it will be swallowed by the nested
REPLs domain. Re-throw the error so that processes don't silently exit
without any indication of an error (including a status code).

Fixes: nodejs#21586
@Trott Trott added the repl Issues and PRs related to the REPL subsystem. label Sep 21, 2018
@nodejs-github-bot
Copy link
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/961/pipeline

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Sep 21, 2018
@Trott
Copy link
Member Author

Trott commented Sep 21, 2018

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

@Trott Trott mentioned this pull request Sep 21, 2018
Closed
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 21, 2018
@jasnell
Copy link
Member

jasnell commented Sep 21, 2018

semver-major because it is surfacing the error.

@Trott Trott added this to the 11.0.0 milestone Sep 21, 2018
@Trott
Copy link
Member Author

Trott commented Sep 21, 2018

semver-major because it is surfacing the error.

In favor of this being patch rather than major: This is a bug that was introduced in Node.js 9.9.0. The bug did not exist in Node.js 9.8.0. (I'm guessing 9.8.0 didn't have the nested REPL thing happening under the hood, so there wasn't a second domain to swallow the error.) So we're restoring previous behavior that was unintentionally altered, rather than surfacing an error that was never surfaced before.

That said, I know semver-ness is a very eye-of-the-beholder thing and I'm fine with this being semver-major, although I'd definitely like to push for it being in 11.0.0 in that case. Adding to the milestone now...

@Trott
Copy link
Member Author

Trott commented Sep 21, 2018 •
edited
Loading

Only failure was a git problem on one of the Windows hosts. I took that host offline temporarily. Rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20926/ ✔️

@jasnell
Copy link
Member

jasnell commented Sep 21, 2018

As always, I'm +1 on patch if there's consensus on TSC. Just labeled it because of policy.

@joyeecheung joyeecheung mentioned this pull request Sep 25, 2018
Closed
@Trott
Copy link
Member Author

Trott commented Sep 27, 2018

/ping @rubys @nodejs/repl @nodejs/tsc

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

This needs more reviews. Already pinged TSC and REPL team, so...going bigger. Sorry. @nodejs/collaborators

benjamingr
benjamingr approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I am +1 on this change and am +1 on semver-patch, this looks like a clear cut bug fix. I will of course defer to others if anyone more believable on our REPL feels strongly about it.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2018

+1 for it to be semver-patch.

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/17586/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2018
@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

Seems to be agreement that this is semver-patch, so I'm going to remove the semver-major label. If anyone disagrees, please re-add the label. Regardless, I wouldn't mind one more @nodejs/tsc review!

@Trott Trott removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 2, 2018
@Trott Trott removed this from the 11.0.0 milestone Oct 2, 2018
ryzokuken
ryzokuken approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

+1 for semver-patch, but I don't think there's much harm in being conservative and landing this as semver-major. I get the point that it's a bugfix, but it does introduce/surface new errors in places it won't throw earlier.

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

+1 for semver-patch, but I don't think there's much harm in being conservative and landing this as semver-major. I get the point that it's a bugfix, but it does introduce/surface new errors in places it won't throw earlier.

We can add dont-land-on labels for 10.x and earlier.

mcollina
mcollina approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Oct 2, 2018
@Trott
repl: do not swallow errors in nested REPLs
83d0404 
For tab completion, a REPLServer instance will sometimes create another
REPLServer instance. If a callback is sent to the `.complete()` function
and that callback throws an error, it will be swallowed by the nested
REPLs domain. Re-throw the error so that processes don't silently exit
without any indication of an error (including a status code).

Fixes: nodejs#21586

PR-URL: nodejs#23004
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

Landed in 83d0404

@Trott Trott closed this Oct 2, 2018
@Trott Trott deleted the fix-21586 branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test-repl-tab-complete.js "params and { on separate line" fails silently.
7 participants
@Trott @nodejs-github-bot @jasnell @BridgeAR @mcollina @benjamingr @ryzokuken