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

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

Closed

Conversation

addaleax
Copy link
Member

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.
@nodejs-github-bot 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 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
microtask_queue() ? microtask_queue().get() : nullptr);
microtask_queue() ?
microtask_queue().get() :
env->isolate()->GetCurrentContext()->GetMicrotaskQueue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env->context()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2020
@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 13, 2020
@github-actions 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
@github-actions
Copy link
Contributor

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

@addaleax
Copy link
Member Author

Landed in d313bf7...c6c8337

@addaleax addaleax closed this Dec 13, 2020
addaleax added a commit that referenced this pull request Dec 13, 2020
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>
addaleax added a commit that referenced this pull request Dec 13, 2020
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>
@addaleax 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
I missed in c6c8337 that we should not just use that queue for
enqueuing microtasks, but also for running them.

Refs: nodejs#36482
addaleax added a commit that referenced this pull request Dec 21, 2020
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>
targos pushed a commit that referenced this pull request Dec 21, 2020
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>
targos pushed a commit that referenced this pull request Dec 21, 2020
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>
targos pushed a commit that referenced this pull request Dec 21, 2020
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>
targos pushed a commit to targos/node that referenced this pull request Jan 8, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Jan 25, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Feb 7, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Feb 11, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Feb 11, 2021
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>
@targos
Copy link
Member

targos commented May 16, 2021

Should it be backported to v14.x?

@targos
Copy link
Member

targos commented May 16, 2021

If that happens, #36581 should be backported too.

addaleax added a commit to addaleax/node that referenced this pull request May 23, 2021
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>
addaleax added a commit to addaleax/node that referenced this pull request May 23, 2021
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>
addaleax added a commit to addaleax/node that referenced this pull request May 23, 2021
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>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
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. embedding Issues and PRs related to embedding Node.js in another project. lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants