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

Jump to bottom

vm: add run-after-evaluate microtask mode #34023

Closed
wants to merge 3 commits into from
Closed

vm: add run-after-evaluate microtask mode #34023

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

@addaleax
vm: add run-after-evaluate microtask mode
35c4401 
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

CI: https://ci.nodejs.org/job/node-test-pull-request/32039/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/32066/

lundibundi
lundibundi approved these changes Jun 26, 2020
View reviewed changes
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.

Sign in to view
@jasnell jasnell mentioned this pull request Jun 26, 2020
Closed
3 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32106/

jasnell pushed a commit that referenced this pull request Jun 26, 2020
@addaleax @jasnell
vm: add run-after-evaluate microtask mode
f63436d 
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@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
@addaleax @MylesBorins
vm: add run-after-evaluate microtask mode
7dffcb3 
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 14, 2020
@MylesBorins
2020-07-15, Version 14.6.0 (Current)
2e91d52 
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
Merged
MylesBorins added a commit that referenced this pull request Jul 15, 2020
@MylesBorins
2020-07-16, Version 14.6.0 (Current)
6629373 
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
@MylesBorins
2020-07-16, Version 14.6.0 (Current)
93694f6 
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
@addaleax @MylesBorins
vm: add run-after-evaluate microtask mode
70c4045 
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 16, 2020
@MylesBorins
2020-07-16, Version 14.6.0 (Current)
35a4b7b 
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
@MylesBorins
2020-07-21, Version 14.6.0 (Current)
049631f 
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
@MylesBorins
2020-07-21, Version 14.6.0 (Current)
7999aa2 
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
@MylesBorins
2020-07-21, Version 14.6.0 (Current)
ca62c34 
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
@MylesBorins
2020-07-21, Version 14.6.0 (Current)
a981c65 
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
@MylesBorins @cjihrig
2020-07-21, Version 14.6.0 (Current)
38678ef 
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
@exoego
Ref. nodejs/node#34023
65f9564
@Raoul1996 Raoul1996 mentioned this pull request Oct 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Promises allow vm.runInContext timeout to be escaped
5 participants
@addaleax @nodejs-github-bot @devsnek @jasnell @lundibundi