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: add run-after-evaluate microtask mode #34023

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

This allows timeouts to apply to e.g. Promises and async functions
from code running inside of vm.Contexts, by giving the Context its
own microtasks queue.

Fixes: #3020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This allows timeouts to apply to e.g. `Promise`s and `async function`s
from code running inside of `vm.Context`s, by giving the Context its
own microtasks queue.

Fixes: nodejs#3020
@addaleax addaleax added vm Issues and PRs related to the vm subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 22, 2020
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 22, 2020
@addaleax
Copy link
Member Author

@nodejs/vm

@addaleax addaleax removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 22, 2020
@nodejs-github-bot
Copy link
Collaborator

@devsnek
Copy link
Member

devsnek commented Jun 22, 2020

Flushing microtasks seems like a good approach, but i'm not sure why that requires MicrotaskQueueWrap to exist or ModuleWrap to have a ContextifyContext instance.

@addaleax
Copy link
Member Author

Flushing microtasks seems like a good approach, but i'm not sure why that requires MicrotaskQueueWrap to exist

Mostly because of the comment in the class definition – there’s no real reason not to give the caller full flexibility over running the microtask queue, it’s just out of scope here.

or ModuleWrap to have a ContextifyContext instance.

It’s absolutely possible that I have missed a simpler way to figure out the relevant microtask queue. If that is the case, please share it with me :)

@devsnek
Copy link
Member

devsnek commented Jun 23, 2020

I'm having trouble thinking of actual use cases for this. You still can't use any stdlib functions from node (or native addons) even if they use microtasks instead of node ticks. afaict anytime you would want to use this option, you should probably be using isolated-vm instead.

@addaleax
Copy link
Member Author

I'm having trouble thinking of actual use cases for this. You still can't use any stdlib functions from node (or native addons) even if they use microtasks instead of node ticks.

Fwiw, multi-context support for Node.js is not totally unthinkable at this point anymore. Quite a few of our internal bindings are set up in a way that would support that by now.

afaict anytime you would want to use this option, you should probably be using isolated-vm instead.

I mean, if you go that route, you might as well say that anybody who wants to use the timeout option should just use isolated-vm (or Worker threads). We have the option, though, and I’m not really happy about that fact, but I do think that this PR enables using it in more scenarios than previously.

@nodejs-github-bot
Copy link
Collaborator

lib/vm.js Show resolved Hide resolved
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Jun 26, 2020
This allows timeouts to apply to e.g. `Promise`s and `async function`s
from code running inside of `vm.Context`s, by giving the Context its
own microtasks queue.

Fixes: #3020

PR-URL: #34023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

Landed in f63436d

@jasnell jasnell closed this Jun 26, 2020
@addaleax addaleax deleted the vm-3020 branch June 26, 2020 18:44
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
This allows timeouts to apply to e.g. `Promise`s and `async function`s
from code running inside of `vm.Context`s, by giving the Context its
own microtasks queue.

Fixes: #3020

PR-URL: #34023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins added a commit that referenced this pull request Jul 14, 2020
Notable changes:

doc:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins added a commit that referenced this pull request Jul 15, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 16, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
This allows timeouts to apply to e.g. `Promise`s and `async function`s
from code running inside of `vm.Context`s, by giving the Context its
own microtasks queue.

Fixes: #3020

PR-URL: #34023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins added a commit that referenced this pull request Jul 16, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 21, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
exoego added a commit to exoego/scala-js-nodejs that referenced this pull request Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promises allow vm.runInContext timeout to be escaped
5 participants