Skip to content

vm: don't print out arrow message for custom error #7398

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

Closed
addaleax wants to merge 5 commits into from
Closed

vm: don't print out arrow message for custom error #7398

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7af968a
vm: don't print out arrow message for custom error
addaleax Jun 24, 2016
06a4039
[squash] add console.error() statement to catch block
addaleax Jun 24, 2016
4f2785d
[squash] use enum to indicate AppendExceptionLine mode
addaleax Jun 24, 2016
1d83e93
[squash] invert and explain arrow-printing logic in AppendExceptionLi…
addaleax Jun 24, 2016
3d9845b
[squash] address review comments
addaleax Jun 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,7 +1423,8 @@ bool IsExceptionDecorated(Environment* env, Local<Value> er) {

void AppendExceptionLine(Environment* env,
Local<Value> er,
Local<Message> message) {
Local<Message> message,
bool handlingFatalError) {
Copy link
Member

@bnoordhuis bnoordhuis Jun 24, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@bnoordhuis bnoordhuis Jun 24, 2016

Choose a reason for hiding this comment

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

Aside: is_fatal_error is a bit more succinct.

Copy link
Member Author

@addaleax addaleax Jun 24, 2016

Choose a reason for hiding this comment

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

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

if (message.IsEmpty())
return;

Expand Down Expand Up @@ -1510,15 +1511,16 @@ void AppendExceptionLine(Environment* env,

Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);

if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) {
if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() &&
(!handlingFatalError || err_obj->IsNativeError())) {
Copy link
Member

@bnoordhuis bnoordhuis Jun 24, 2016

Choose a reason for hiding this comment

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

This is hard to parse. It'd be better to write it out like (for example) this:

if (!(handling_fatal_error || arrow_str.IsEmpty() || err_obj.IsEmpty() || !err->obj->IsNativeError())) {

Still not great but arguably more legible.

Copy link
Contributor

@cjihrig cjihrig Jun 24, 2016

Choose a reason for hiding this comment

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

Would it work to drop the new handling_fatal_error part and do:

if (!arrow_str.IsEmpty() && !err_obj.IsEmpty()) {
  if (err_obj->IsNativeError()) {
    // err_obj->SetPrivate(...);
  }

  return;
}

That way the printing part would only be hit if allocation failed.

Copy link
Member Author

@addaleax addaleax Jun 24, 2016

Choose a reason for hiding this comment

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

@cjihrig Probably not. I thought about that, but the current behaviour of printing for non-native errors is actually tested.

Printing out in that case also makes sense, because otherwise the user would receive no indicator that the output is actually an error, i.e. node could fail with just [Object object] as its output.

@bnoordhuis That would not have the same semantics, though. I don’t want to blow up the diff too much, but I think the actual more readable thing to do would be inverting the code paths, i.e. moving the printing inside a conditional to indicate that that is not the default route.

Copy link
Member

@bnoordhuis bnoordhuis Jun 24, 2016

Choose a reason for hiding this comment

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

If I got the semantics wrong, then that's probably good indication that the current clause is hard to understand. :-)

Copy link
Member Author

@addaleax addaleax Jun 24, 2016

Choose a reason for hiding this comment

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

@bnoordhuis Definitely not disagreeing with that. :) I know you’re not a fan of comments when the code can be self-explanatory, but I’ve still added a commit that inverts the logic + a comment that goes through the various scenarios. I think I would have found that helpful when looking at the code, but if you don’t like it or have different suggestions, I’m happy to hear that.

err_obj->SetPrivate(
env->context(),
env->arrow_message_private_symbol(),
arrow_str);
return;
}

// Allocation failed, just print it out.
// Allocation failed when called from ReportException(), just print it out.
if (env->printed_error())
return;
env->set_printed_error(true);
Expand All @@ -1532,7 +1534,7 @@ static void ReportException(Environment* env,
Local<Message> message) {
HandleScope scope(env->isolate());

AppendExceptionLine(env, er, message);
AppendExceptionLine(env, er, message, true);

Local<Value> trace_value;
Local<Value> arrow;
Expand Down
3 changes: 2 additions & 1 deletion src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ bool IsExceptionDecorated(Environment* env, v8::Local<v8::Value> er);

void AppendExceptionLine(Environment* env,
v8::Local<v8::Value> er,
v8::Local<v8::Message> message);
v8::Local<v8::Message> message,
bool handlingFatalError = false);

NO_RETURN void FatalError(const char* location, const char* message);

Expand Down
18 changes: 18 additions & 0 deletions test/message/vm_caught_custom_runtime_error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
require('../common');
const vm = require('vm');

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.
Copy link
Contributor

@cjihrig cjihrig Jun 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@addaleax addaleax Jun 24, 2016

Choose a reason for hiding this comment

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

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

try {
vm.runInThisContext(`throw ({
name: 'MyCustomError',
message: 'This is a custom message'
})`, { filename: 'test.vm' });
} catch (e) {
console.error('received error', e.name);
}
Copy link
Contributor

@Fishrock123 Fishrock123 Jun 24, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@addaleax addaleax Jun 24, 2016

Choose a reason for hiding this comment

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

@Fishrock123 sounds good, done


console.error('end');
3 changes: 3 additions & 0 deletions test/message/vm_caught_custom_runtime_error.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
beginning
received error MyCustomError
end