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.

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

displayErrors option of the vm module seems not to work well #4835

Closed
susisu opened this issue Jan 24, 2016 · 7 comments
Closed

displayErrors option of the vm module seems not to work well #4835

susisu opened this issue Jan 24, 2016 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.

Comments

@susisu
Copy link

susisu commented Jan 24, 2016

I'm stuck for the behavior of the displayErrors option of the vm module.

First I wrote the code like below and this works well.

var vm = require("vm");
var sandbox = Object.create(null);
var context = vm.createContext(sandbox);
var script  = new vm.Script("?", { filename: "foobar", displayErrors: false });
script.runInContext(context, { filename: "foobar", displayErrors: false });
foobar:1
?
^

SyntaxError: Unexpected token ?
    at Object.<anonymous> (/path/to/file.js:4:15)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:139:18)
    at node.js:999:3

When displayErrors are set to true, however, the behavior seems to be not consistent with the document "print any errors to stderr before throwing an exception" since this code does not output anything to stderr.

var vm = require("vm");
var sandbox = Object.create(null);
var context = vm.createContext(sandbox);
var script  = new vm.Script("?", { filename: "foobar", displayErrors: true });
script.runInContext(context, { filename: "foobar", displayErrors: true });
foobar:1
?
^

SyntaxError: Unexpected token ?
    at Object.<anonymous> (/Users/Susisu/root/projects/est/dev/vmtest.js:4:15)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:139:18)
    at node.js:999:3

Next, I tried to catch the error and get the information where the error has occurred but I couldn't.

I also re-threw the error and found that the information about the script is discarded when the error is re-thrown if displayErrors: false, while the displayErrors: true version retains it.

var vm = require("vm");
var sandbox = Object.create(null);
var context = vm.createContext(sandbox);
try {
    var script  = new vm.Script("?", { filename: "foobar", displayErrors: false });
    script.runInContext(context, { filename: "foobar", displayErrors: false });
}
catch (err) {
    throw err;
}
/path/to/file.js:9
    throw err;
    ^

SyntaxError: Unexpected token ?
    at Object.<anonymous> (/path/to/file.js:5:19)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:139:18)
    at node.js:999:3
var vm = require("vm");
var sandbox = Object.create(null);
var context = vm.createContext(sandbox);
try {
    var script  = new vm.Script("?", { filename: "foobar", displayErrors: true });
    script.runInContext(context, { filename: "foobar", displayErrors: true });
}
catch (err) {
    throw err;
}
foobar:1
?
^

SyntaxError: Unexpected token ?
    at Object.<anonymous> (/path/to/file.js:5:19)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:139:18)
    at node.js:999:3

The problem is more serious when combined with Promise.
This code actually does not print anything since the error is swallowed by the Promise, and I have no idea to know where the error occurred in the script.

var vm = require("vm");
new Promise((resolve, reject) => {
    var sandbox = Object.create(null);
    var context = vm.createContext(sandbox);
    var script  = new vm.Script("?", { filename: "foobar", displayErrors: false });
    script.runInContext(context, { filename: "foobar", displayErrors: false });
}).catch(err => {
    // can't do anything
});

Note that all the codes throw syntax errors, but the results are almost same for throwing runtime errors.

Taken together, it seems that displayErrors: true does only make the error retain the information where it occurred and does not let new vm.Script (and runInContext or something) print anything.
In addition, there seems to be no way to know where the error occurred if it is in Promise.

Here I have two questions.

  1. Is this behavior of displayErrors: true is correct?
  2. Are there some ways to know where the error occurred in the script for the last case?

The version of Node.js is v5.5.0.

Thanks.

@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem. labels Jan 24, 2016
@bnoordhuis
Copy link
Member

I had a look and I can confirm what you're describing. What happens is this:

  1. The SyntaxError that V8 creates doesn't point to the source location of the invalid syntax.
  2. What node.js does is attach a hidden property to the exception object that contains the necessary information.

Example:

var vm = require('vm');
var util = require('internal/util');  // requires --expose_internals
try {
  new vm.Script('?', {filename: 'x.js', displayErrors: true});
} catch (e) {
  console.error('arrowMessage:', util.getHiddenValue(e, 'node:arrowMessage'));
}

Which prints:

$ node --expose_internals t.js
arrowMessage: x.js:1
?
^

If you let the exception bubble up to node's top-level exception handler, it will include the arrowMessage string in the output. With that in mind:

Is this behavior of displayErrors: true is correct?

It's as correct as it can be given the circumstances but it's at odds with the documentation. We could either restore the old behavior where node prints the message immediately on syntax error or we could update the documentation.

If we update the documentation, there probably needs to be an official way to get the hidden property, which is awkward.

Are there some ways to know where the error occurred in the script for the last case?

Nothing official at the moment, no, except for letting the exception bubble up. (EDIT: Which doesn't work for promises, of course.)

/cc @nodejs/documentation @cjihrig

@benjamingr
Copy link
Member

EDIT: Which doesn't work for promises, of course.

Why?

@bnoordhuis
Copy link
Member

The exception won't bubble up all the way, at least not by default.

You could add a process.on('unhandledRejection', err => { throw err }) and drop any .catch(...) handlers from your promises but that's not very elegant and not always workable.

@benjamingr
Copy link
Member

Well, if you have .catch handlers that's the same as not letting exceptions bubble (by not having a synchronous catch(e) {). Adding on('unahndledRejection', err => { throw err; }) is something you probably want to do when using vm.

I get your point though - thanks.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 24, 2016

My guess is that this behavior goes back to f1de13b, which fixes nodejs/node-v0.x-archive#6920.

@bnoordhuis I'm happy to work on this if we decide how we want to fix it.

@susisu
Copy link
Author

susisu commented Jan 24, 2016

@bnoordhuis Thank you for your answer. I see the point.

I will try to use process.on('unhandledRejection', ...) for this case for now.

@bnoordhuis
Copy link
Member

@cjihrig The best I can come up with is to intercept the SyntaxError and add the extra information to the .message property instead of a hidden property. That's pretty yuck, though.

cjihrig added a commit to cjihrig/node that referenced this issue Jan 26, 2016
The vm module's displayErrors option attaches error arrow
messages as a hidden property. Later, core JavaScript code
can optionally decorate the error stack with the arrow message.
However, when user code catches an error, it has no way to
access the arrow message. This commit changes the behavior of
displayErrors to mean "decorate the error stack if an error
occurs."

Fixes: nodejs#4835
benjamingr pushed a commit to benjamingr/io.js that referenced this issue Jan 27, 2016
The vm module's displayErrors option attaches error arrow
messages as a hidden property. Later, core JavaScript code
can optionally decorate the error stack with the arrow message.
However, when user code catches an error, it has no way to
access the arrow message. This commit changes the behavior of
displayErrors to mean "decorate the error stack if an error
occurs."

Fixes: nodejs#4835
PR-URL: nodejs#4874
Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
The vm module's displayErrors option attaches error arrow
messages as a hidden property. Later, core JavaScript code
can optionally decorate the error stack with the arrow message.
However, when user code catches an error, it has no way to
access the arrow message. This commit changes the behavior of
displayErrors to mean "decorate the error stack if an error
occurs."

Fixes: nodejs#4835
PR-URL: nodejs#4874
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants