src: use correct outer Context’s microtask queue by addaleax · Pull Request #36482 · nodejs/node · GitHub

deps: V8: backport 4bf051d536a1

Original commit message:

[api] Add Context::GetMicrotaskQueue method

Add a method that returns the microtask queue that is being used
by the `v8::Context`.

This is helpful in non-monolithic embedders like Node.js, which
accept Contexts created by its own embedders like Electron, or
for native Node.js addons. In particular, it enables:

1. Making sure that “nested” `Context`s use the correct microtask
   queue, i.e. the one from the outer Context.
2. Enqueueing microtasks into the correct microtask queue.

Previously, these things only worked when the microtask queue for
a given Context was the Isolate’s default queue.

As an alternative, I considered adding a way to make new `Context`s
inherit the queue from the `Context` that was entered at the time
of their creation, but that seemed a bit more “magic”, less flexible,
and didn’t take care of concern 2 listed above.

Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71710}

Refs: v8/v8@4bf051d

src: use correct outer Context’s microtask queue

Fall back to using the outer context’s microtask queue, rather than
the Isolate’s default one. This would otherwise result in surprising
behavior if an embedder specified a custom microtask queue for the
main Node.js context.

Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71710} Refs: v8/v8@4bf051d

Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context.

env->context()?

That's equivalent right now, yes, but I'd like to write this in a more future-proof way if you don't mind :)

There will be situations where the active environment doesn't match the active context?

Well, we could extend Node.js to supporting multiple different Contexts per Environment, which would be nice, and we’re not that far from supporting it

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

Commit Queue failed

- Loading data for nodejs/node/pull/36482
✔  Done loading data for nodejs/node/pull/36482
----------------------------------- PR info ------------------------------------
Title      src: use correct outer Context’s microtask queue (#36482)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     addaleax:correct-microtaskqueue-nesting -> nodejs:master
Labels     V8 Engine, author ready, embedding, lib / src
Commits    3
 - deps: V8: backport 4bf051d536a1
 - src: use correct outer Context’s microtask queue
 - fixup! src: use correct outer Context’s microtask queue
Committers 1
 - Anna Henningsen 
PR-URL: https://github.com/nodejs/node/pull/36482
Refs: https://github.com/v8/v8/commit/4bf051d536a172e7932845d82f918ad7280c7873
Reviewed-By: Gus Caplan 
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36482
Refs: https://github.com/v8/v8/commit/4bf051d536a172e7932845d82f918ad7280c7873
Reviewed-By: Gus Caplan 
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-12-11T16:52:38Z: https://ci.nodejs.org/job/node-test-pull-request/34903/
- Querying data for job/node-test-pull-request/34903/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Fri, 11 Dec 2020 16:17:01 GMT
   ✔  Approvals: 3
   ✔  - Gus Caplan (@devsnek): https://github.com/nodejs/node/pull/36482#pullrequestreview-550252608
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/36482#pullrequestreview-550306426
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36482#pullrequestreview-550925762
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/419379169

Landed in d313bf7...c6c8337

Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71710} Refs: v8/v8@4bf051d PR-URL: #36482 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context. PR-URL: #36482 Refs: v8/v8@4bf051d Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

I missed in c6c8337 that we should not just use that queue for enqueuing microtasks, but also for running them. Refs: nodejs#36482

I missed in c6c8337 that we should not just use that queue for enqueuing microtasks, but also for running them. Refs: #36482 PR-URL: #36581 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71710} Refs: v8/v8@4bf051d PR-URL: #36482 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context. PR-URL: #36482 Refs: v8/v8@4bf051d Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

I missed in c6c8337 that we should not just use that queue for enqueuing microtasks, but also for running them. Refs: #36482 PR-URL: #36581 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71710} Refs: v8/v8@4bf051d PR-URL: nodejs#36482 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Should it be backported to v14.x?

If that happens, #36581 should be backported too.

Original commit message: [api] Add Context::GetMicrotaskQueue method Add a method that returns the microtask queue that is being used by the `v8::Context`. This is helpful in non-monolithic embedders like Node.js, which accept Contexts created by its own embedders like Electron, or for native Node.js addons. In particular, it enables: 1. Making sure that “nested” `Context`s use the correct microtask queue, i.e. the one from the outer Context. 2. Enqueueing microtasks into the correct microtask queue. Previously, these things only worked when the microtask queue for a given Context was the Isolate’s default queue. As an alternative, I considered adding a way to make new `Context`s inherit the queue from the `Context` that was entered at the time of their creation, but that seemed a bit more “magic”, less flexible, and didn’t take care of concern 2 listed above. Change-Id: I15ed796df90f23c97a545a8e1b30a3bf4a5c4320 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2579914 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#71710} Refs: v8/v8@4bf051d PR-URL: nodejs#36482 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context. PR-URL: nodejs#36482 Refs: v8/v8@4bf051d Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

I missed in c6c8337 that we should not just use that queue for enqueuing microtasks, but also for running them. Refs: nodejs#36482 PR-URL: nodejs#36581 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

addaleax added 2 commits December 11, 2020 17:16

src: use correct outer Context’s microtask queue

299c63d

Fall back to using the outer context’s microtask queue, rather than the Isolate’s default one. This would otherwise result in surprising behavior if an embedder specified a custom microtask queue for the main Node.js context.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 11, 2020

addaleax added embedding Issues and PRs related to embedding Node.js in another project. request-ci Add this label to start a Jenkins CI on a PR. v8 engine Issues and PRs related to the V8 dependency. labels Dec 11, 2020

fixup! src: use correct outer Context’s microtask queue

bde7245

devsnek approved these changes Dec 11, 2020

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020

jasnell approved these changes Dec 11, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2020

Trott approved these changes Dec 13, 2020

View reviewed changes

addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 13, 2020

github-actions Bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 13, 2020

addaleax closed this Dec 13, 2020

addaleax deleted the correct-microtaskqueue-nesting branch December 13, 2020 21:04

addaleax added a commit to addaleax/node that referenced this pull request Dec 19, 2020

src: use correct microtask queue for checkpoints

4bcbd46

I missed in c6c8337 that we should not just use that queue for enqueuing microtasks, but also for running them. Refs: nodejs#36482

addaleax mentioned this pull request Dec 19, 2020

src: use correct microtask queue for checkpoints #36581

Closed

3 tasks

targos mentioned this pull request Dec 22, 2020

v15.5.0 release proposal #36597

Merged

targos added the backport-requested-v14.x label May 16, 2021

addaleax mentioned this pull request May 23, 2021

[v14.x] src: use correct outer Context’s microtask queue #38787

Closed

addaleax added backport-open-v14.x and removed backport-requested-v14.x labels May 23, 2021

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

targos removed the backport-open-v14.x label May 3, 2023

-                    microtask_queue() ? microtask_queue().get() : nullptr);
+                    microtask_queue() ?
+                        microtask_queue().get() :
+                        env->isolate()->GetCurrentContext()->GetMicrotaskQueue());

src: use correct outer Context’s microtask queue by addaleax · Pull Request #36482 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: use correct outer Context’s microtask queue - #36482

src: use correct outer Context’s microtask queue#36482
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:correct-microtaskqueue-nesting

addaleax commented Dec 11, 2020

Uh oh!

devsnek Dec 11, 2020

Uh oh!

addaleax Dec 11, 2020

Uh oh!

devsnek Dec 11, 2020

Uh oh!

addaleax Dec 11, 2020

Uh oh!

nodejs-github-bot commented Dec 11, 2020

Uh oh!

github-actions Bot commented Dec 13, 2020

Uh oh!

addaleax commented Dec 13, 2020

Uh oh!

targos commented May 16, 2021

Uh oh!

targos commented May 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: use correct outer Context’s microtask queue by addaleax · Pull Request #36482 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: use correct outer Context’s microtask queue - #36482

src: use correct outer Context’s microtask queue#36482addaleax wants to merge 3 commits intonodejs:masternodejs/node:masterfrom addaleax:correct-microtaskqueue-nestingaddaleax/node:correct-microtaskqueue-nestingCopy head branch name to clipboard

Conversation

addaleax commented Dec 11, 2020

deps: V8: backport 4bf051d536a1

src: use correct outer Context’s microtask queue

Uh oh!

devsnek Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

devsnek Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 11, 2020

Uh oh!

github-actions Bot commented Dec 13, 2020

Uh oh!

addaleax commented Dec 13, 2020

Uh oh!

targos commented May 16, 2021

Uh oh!

targos commented May 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: use correct outer Context’s microtask queue#36482
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:correct-microtaskqueue-nesting