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

Handle errors thrown in async functions #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bespokebob
Copy link
Contributor

If a callback passed to setTimeout (and other timer functions) throws an error, the error propogates up through the host node process, potentially crashing it unless process.on('uncaughtException') is used.

Wrap all callback functions passed to the various host timer functions in a try/catch, so errors are handled and emitted as events on the vm.

Handle several cases where a promise could end up in an unhandledRejection state (which will cause the process to exit in a future version of Node).

This is a rewrite of #213 to work with the new sandbox code.

If a callback passed to setTimeout (and other timer functions) throws an error, the error propogates up through the host node process, potentially crashing it unless process.on('uncaughtException') is used.

Wrap all callback functions passed to the various host timer functions in a try/catch, so errors are handled and emitted as events on the vm.

Handle several cases where a promise could end up in an unhandledRejection state (which will cause the process to exit in a future version of Node).
@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 6, 2022

I do not like the Promise case. As far as I can see it does not handle all cases and in my opinion node did a bad decision to exit the node process when encountering an unhandled promise as:

const p = new Promise((_,r)=>r(new Error("")));
setTimeout(()=>p.catch(console.log), 1000);

is totally valid JavaScript which should run without termination and no unhandled promise warning (see https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-host-promise-rejection-tracker Note 1).

Use the process unhandledRejection listener or command line flag --unhandled-rejections=warn instead.

@bespokebob
Copy link
Contributor Author

I don't really have control over node's decisions here, I just need to live with them.

I realize I can catch these errors at the process level, but I would really like some way to differentiate between any "host" process errors and any errors happening inside NodeVM. It's not possible to call process.exit() inside NodeVM to kill the host process, so it seems to me like letting errors and unhandled rejections bubble up is giving the sandbox an inappropriate way to affect the parent process (by potentially killing it, unless worked around in some way).

I directly ported this change over from an older version of vm2 before some of your refactoring work, and I honestly don't understand how a lot of it is working, so it would not surprise me if there was a better way to handle this. I think the test cases are valid, though. Without the sandbox changes, the test cases fail with no way to easily catch the errors (since they happen asynchronously).

At the very least, something should be added to the error handling section of the documentation about this (it currently only mentions uncaughtException and nothing about the fact that unhandled rejections can crash the process from NodeVM).

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 6, 2022

by potentially killing it, unless worked around in some way

Killing the process was always possible with memory consumption FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory.

@bespokebob
Copy link
Contributor Author

by potentially killing it, unless worked around in some way

Killing the process was always possible with memory consumption FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory.

And there are probably other ways, but the goal of this project seems to be to avoid sandbox breakouts and affecting the parent process, and this seems to be an avoidable cause. I'm not sure why that would be a good reason for not trying to solve this.

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 6, 2022

I would like to include this. However, I fear that for the promise case this would cause confusion. One might wonder why

(async function f() {
  throw new Error();
})();

is not handled but only the promise case. In my opinion either all cases should be handled or none. If there is a line in-between it is hard to communicate this line so that there will be no confusion. And since handling all cases is very hard, I do not see the gain to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants