Skip to content

repl: handle null thrown - #14306

Closed
benjamingr wants to merge 2 commits into
nodejs:masterfrom
benjamingr:repl-handle-null-thrown
Closed

repl: handle null thrown#14306
benjamingr wants to merge 2 commits into
nodejs:masterfrom
benjamingr:repl-handle-null-thrown

Conversation

@benjamingr

@benjamingrbenjamingr commented Jul 16, 2017•
edited
Loading

Copy link
Copy Markdown
Member
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Fixes#12373

@nodejs-github-botnodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 16, 2017
benjamingr
benjamingr commented Jul 16, 2017
View reviewed changes
Comment threadtest/parallel/test-repl-sigint.js Outdated

@benjamingrbenjamingrJul 16, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

whoops

@benjamingrbenjamingr mentioned this pull request Jul 17, 2017
Closed
TimothyGu
TimothyGu reviewed Jul 17, 2017
View reviewed changes
Comment threadlib/repl.js Outdated

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, things that are thrown aren't necessarily always errors. Maybe Thrown: or just ${e}\n?

(Also does this work with symbols?)

@benjamingrbenjamingrJul 17, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@TimothyGu I preferred to make the smaller changed - for it to work with symbols we'd need to wrap it in String (I can do that).

I can change the terminology from Error: to Thrown: I was hoping we could get a little bikeshedding on the message.

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah well, this kinda goes back to #13784, which is still in limbo because of the lack of a good way to stringify an object.

@benjamingrbenjamingrJul 17, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@TimothyGu yes, but I think String is a good middle ground - I don't want it to be bulletproof - I want it to be useful for users.

TimothyGu
TimothyGu reviewed Jul 17, 2017
View reviewed changes
Comment threadlib/repl.js Outdated

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah well, this kinda goes back to #13784, which is still in limbo because of the lack of a good way to stringify an object.

Comment threadtest/parallel/test-repl-null-thrown.js Outdated

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the process.nextTick necessary?

@benjamingrbenjamingrJul 17, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@TimothyGu yes, to reproduce the issue - without the nextTick it doesn't crash the REPL.

Benjamin Gruenbaum
repl: don't terminate on null thrown
4dfe7fd
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.
PR-URL: nodejs#14306Fixes: nodejs#12373
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@benjamingr
benjamingrforce-pushed the repl-handle-null-thrown branch from 3200c5a to 4dfe7fdCompareJuly 17, 2017 08:18
@benjamingr

benjamingr commented Jul 17, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu renamed "Error: " to "Thrown: " and handled thrown symbols.

TimothyGu
TimothyGu approved these changes Jul 17, 2017
View reviewed changes

@TimothyGuTimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Comment threadlib/repl.js
const top = replMap.get(self);

internalUtil.decorateErrorStack(e);
const isError = internalUtil.isError(e);

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not process.binding('util').isNativeError?

@benjamingrbenjamingrJul 17, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@TimothyGu Not sure that's the behaviour I'd want - I'd like people to (optimally) be able to override Symbol.toStringTag (or even the name) and take the "Error like" path if they really need to.

I don't feel strongly about this though. If you do - let me know and I'll change it.

@TimothyGuTimothyGuJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good to me.

jasnell
jasnell reviewed Jul 17, 2017
View reviewed changes
Comment threadtest/parallel/test-repl-null-thrown.js
replserver.emit('line', '.exit');

setTimeout(() => {
console.log(text);

@jasnelljasnellJul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

extraneous console.log() output?

Fishrock123
Fishrock123 approved these changes Jul 17, 2017
View reviewed changes

@Fishrock123Fishrock123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

cjihrig
cjihrig approved these changes Jul 17, 2017
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I am curious why people aren't asking for all of the obnoxious scenarios from #12400 though :-D

@TimothyGu

TimothyGu commented Jul 18, 2017

Copy link
Copy Markdown
Member

I mean, we can still craft weird scenarios with @@toStringTag, @@toPrimitive, valueOf, toString, stack, Proxys, etc. I know I've fallen into the traps of bikeshedding before (sorry @cjihrig), but this seems like as KISS of a threshold as any. And I guess the user deserves to get some weird error if they does one or more of those things.

@TimothyGu

TimothyGu commented Jul 19, 2017

Copy link
Copy Markdown
Member

Landed in 380e814.

@benjamingr

benjamingr commented Jul 19, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu note: you landed this without passing CI (last CI was failing due to changed error message). I've started a CI in https://ci.nodejs.org/job/node-test-pull-request/9246/console

@TimothyGu

TimothyGu commented Jul 19, 2017

Copy link
Copy Markdown
Member

@benjamingr I didn't land this BTW, I just noticed it was landed w/o a "landed" comment.

@benjamingr

benjamingr commented Jul 19, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu I landed and immediately force reverted because of the CI (and posted in #node-dev) - currently waiting for CI to land this.

@benjamingrbenjamingr reopened this Jul 19, 2017
@TimothyGu

TimothyGu commented Jul 19, 2017

Copy link
Copy Markdown
Member

@benjamingr Ah oops. Sorry, should've let you handle this (and checked IRC...)

@benjamingr

benjamingr commented Jul 19, 2017

Copy link
Copy Markdown
MemberAuthor

ARM fanned failure on inspector/test-inspector-port-cluster unrelated.

@benjamingr

benjamingr commented Jul 19, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in 030a028 🎉

benjamingr pushed a commit that referenced this pull request Jul 19, 2017
Benjamin Gruenbaum
repl: don't terminate on null thrown
030a028
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.
PR-URL: #14306Fixes: #12373
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com
addaleax pushed a commit that referenced this pull request Jul 22, 2017
Benjamin Gruenbaum@addaleax
repl: don't terminate on null thrown
ef3139a
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.
PR-URL: #14306Fixes: #12373
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
Benjamin Gruenbaum@Fishrock123
repl: don't terminate on null thrown
34821f6
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.
PR-URL: #14306Fixes: #12373
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com
@addaleaxaddaleax mentioned this pull request Jul 24, 2017
Closed
@addaleaxaddaleax mentioned this pull request Aug 2, 2017
Merged
@MylesBorins

MylesBorins commented Aug 16, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@IonicaBizauIonicaBizau mentioned this pull request Oct 27, 2017
Closed
@MylesBorins

MylesBorins commented Dec 19, 2017

Copy link
Copy Markdown
Contributor

ping @benjamingr

1 similar comment
@MylesBorins

MylesBorins commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

ping @benjamingr

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+2 more reviewers
@TimothyGuTimothyGuTimothyGu approved these changes
@Fishrock123Fishrock123Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

replIssues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@benjamingr@TimothyGu@MylesBorins@jasnell@addaleax@Fishrock123@cjihrig@nodejs-github-bot