vm: don't print out arrow message for custom error by addaleax · Pull Request #7398 · nodejs/node

addaleax · 2016-06-24T03:53:24Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

vm

Description of change

In AppendExceptionLine(), which is used both by the vm
module and the uncaught exception handler, don’t print anything
to stderr when called from the vm module, even if the
thrown object is not a native error instance.

Fixes: #7397

In `AppendExceptionLine()`, which is used both by the `vm` module and the uncaught exception handler, don’t print anything to stderr when called from the `vm` module, even if the thrown object is not a native error instance. Fixes: nodejs#7397

Fishrock123 · 2016-06-24T08:39:00Z

test/message/vm_caught_custom_runtime_error.js

+    message: 'This is a custom message'
+  })`, { filename: 'test.vm' });
+} catch (e) {
+}


Should we also do another print from the catch to make sure it routes that way?

@Fishrock123 sounds good, done

bnoordhuis · 2016-06-24T09:58:03Z

src/node.cc

                         Local<Value> er,
-                         Local<Message> message) {
+                         Local<Message> message,
+                         bool handlingFatalError) {


Style: snake_case, not camelCase for parameters (i.e., handling_fatal_error.)

Also, an enum might be nicer because it gives a clear hint what the parameter does at the call site. With bools, I usually use a local variable to make it obvious:

const bool handling_fatal_error = true; AppendExceptionLine(env, err, message, handling_fatal_error);

Aside: is_fatal_error is a bit more succinct.

I’ve updated with an enum, that’s a good idea.

…ne()

bnoordhuis · 2016-06-24T15:12:02Z

src/node.cc

                         Local<Value> er,
-                         Local<Message> message) {
+                         Local<Message> message,
+                         enum ErrorHandlingMode mode) {


The enum keyword is not strictly necessary (but it doesn't hurt and I realize we use it in other places.)

bnoordhuis · 2016-06-24T15:12:44Z

LGTM with comments.

cjihrig · 2016-06-24T15:16:22Z

test/message/vm_caught_custom_runtime_error.js

+console.error('beginning');
+
+// Regression test for https://github.com/nodejs/node/issues/7397:
+// This should not print out anything to stderr due to the error being caught.


This is the tiniest of nits, but maybe say vm.runInThisContext() should not print anything... As it currently stands, it seems like nothing should be printed at all, or the catch won't print anything either.

@cjihrig Done, forgot to update this along with the catch block

cjihrig · 2016-06-24T15:16:33Z

LGTM I suppose.

addaleax · 2016-06-25T03:17:10Z

CI: https://ci.nodejs.org/job/node-test-commit/3846/

addaleax · 2016-06-30T11:04:05Z

Landed in 3cac616

In `AppendExceptionLine()`, which is used both by the `vm` module and the uncaught exception handler, don’t print anything to stderr when called from the `vm` module, even if the thrown object is not a native error instance. Fixes: #7397 PR-URL: #7398 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-07-11T21:18:32Z

@addaleax this is not landing cleanly, would you be willing to backport to v4.x?

In `AppendExceptionLine()`, which is used both by the `vm` module and the uncaught exception handler, don’t print anything to stderr when called from the `vm` module, even if the thrown object is not a native error instance. Fixes: nodejs#7397 PR-URL: nodejs#7398 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

In `AppendExceptionLine()`, which is used both by the `vm` module and the uncaught exception handler, don’t print anything to stderr when called from the `vm` module, even if the thrown object is not a native error instance. Fixes: #7397 PR-URL: #7398 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Jun 24, 2016

addaleax mentioned this pull request Jun 24, 2016

Eval and vm module with custom errors #7397

Closed

Fishrock123 reviewed Jun 24, 2016
View reviewed changes

[squash] add console.error() statement to catch block

06a4039

bnoordhuis reviewed Jun 24, 2016
View reviewed changes

addaleax added 2 commits June 24, 2016 16:29

[squash] use enum to indicate AppendExceptionLine mode

4f2785d

[squash] invert and explain arrow-printing logic in AppendExceptionLi…

1d83e93

…ne()

bnoordhuis reviewed Jun 24, 2016
View reviewed changes

cjihrig reviewed Jun 24, 2016
View reviewed changes

[squash] address review comments

3d9845b

addaleax closed this Jun 30, 2016

addaleax deleted the fix-7397 branch June 30, 2016 11:04

addaleax added the lts-watch-v4.x label Jun 30, 2016

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

addaleax mentioned this pull request Jul 11, 2016

vm: don't print out arrow message for custom error (v4.x) #7664

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Uh oh!

Conversation

addaleax commented Jun 24, 2016

Checklist

Affected core subsystem(s)

Description of change

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jun 24, 2016

Uh oh!

addaleax commented Jun 25, 2016

Uh oh!

addaleax commented Jun 30, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants