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

vm.Script should support timeouts #20982

Closed
SimenB opened this issue May 26, 2018 · 3 comments
Closed

vm.Script should support timeouts #20982

SimenB opened this issue May 26, 2018 · 3 comments
Labels
doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.

Comments

@SimenB
Copy link
Member

SimenB commented May 26, 2018

  • Version: 10.2.1
  • Platform: Darwin Simens-MacBook-Pro.local 17.4.0 Darwin Kernel Version 17.4.0: Sun Dec 17 09:19:54 PST 2017; root:xnu-4570.41.2~1/RELEASE_X86_64 x86_64
  • Subsystem: vm

According to the docs, you should be able to pass timeout when doing new Script. This seemingly does not work.

Running the following snippet will successfully throw an error after one second:

const vm = require('vm');

const script = new vm.Script('while(true){}');

script.runInContext(vm.createContext(), {timeout: 1000});

The following will not:

const vm = require('vm');

const script = new vm.Script('while(true){}', {timeout: 1000});

script.runInContext(vm.createContext());

I did some digging (which might not be useful at all), and to my untrained eyes it looks like new Script constructor ignores timeout argument:

node/lib/vm.js

Lines 45 to 52 in 9461f32

const {
filename = 'evalmachine.<anonymous>',
lineOffset = 0,
columnOffset = 0,
cachedData,
produceCachedData = false,
[kParsingContext]: parsingContext
} = options;

Reported in jsdom/jsdom#2238, /cc @TimothyGu

@devsnek
Copy link
Member

devsnek commented May 26, 2018

the docs are wrong in this case (displayErrors should also not be listed), would you care to open a pr? its fine if not.

@devsnek devsnek added doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem. labels May 26, 2018
@SimenB
Copy link
Member Author

SimenB commented May 26, 2018

That's a shame!

Sure, I can send a PR for that

SimenB added a commit to SimenB/node that referenced this issue May 26, 2018
@SimenB
Copy link
Member Author

SimenB commented May 26, 2018

See #20984

MylesBorins pushed a commit that referenced this issue Jun 6, 2018
PR-URL: #20984
Fixes: #20982
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants