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.

Sign up for GitHub

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

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

Closed
wants to merge 66 commits into from
Closed

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

wants to merge 66 commits into from

Conversation

addaleax
Copy link
Member

Backports #36482 and #36581 to v14.x. Only conflicts were neighbouring-line conflicts in the V8 cctest file and common.gypi, so I don’t think this needs actual review, just CI.

targos and others added 30 commits May 16, 2021 15:10
@targos
tools: add new ESLint rule: prefer-primordials
5962d42 
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: nodejs#35448
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@aduh95 @targos
tools,lib: tighten prefer-primordials rules for Error statics
5068db6 
PR-URL: nodejs#36017
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 @targos
tools,lib: recommend using safe primordials
1687197 
Make the linter recommend replacing `globalThis.Map` by
`primordials.SafeMap`, and similar for `Set`, `WeakSet`, and `WeakMap`.

PR-URL: nodejs#36026
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
@targos
timers: cleanup abort listener on awaitable timers
538ec98 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@ExE-Boss @targos
lib: add %TypedArray% abstract constructor to primordials
2fd37db 
Refs: nodejs#35448
Refs: nodejs#36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#36016
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
@aduh95 @targos
module: refactor to use more primordials
1f38c63 
PR-URL: nodejs#36024
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@cjihrig @targos
tools: update ESLint to 7.11.0
64c5f62 
Update ESLint to 7.11.0

PR-URL: nodejs#35578
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@cjihrig @targos
tools: update ESLint to 7.12.0
8b2df0c 
Update ESLint to 7.12.0

PR-URL: nodejs#35799
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@cjihrig @targos
tools: update ESLint to 7.12.1
8e4a055 
Update ESLint to 7.12.1

PR-URL: nodejs#35799
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@lpinca @targos
tools: update ESLint to 7.13.0
17c8484 
Update ESLint to 7.13.0

PR-URL: nodejs#36031
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax @targos
test: use .then(common.mustCall()) for all async IIFEs
845f3ae 
This makes sure that all async functions finish as expected.

PR-URL: nodejs#34363
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax @targos
tools: add linting rule for async IIFEs
02233af 
The result of an async IIFE should always be handled in our tests,
typically by adding `.then(common.mustCall())` to verify that the
async function actually finishes executing at some point.

PR-URL: nodejs#34363
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@mmarchini @targos
test: add Actions annotation output
1f3fff3 
It's possible to annotate failures in Actions by printing
"::error file={},line={},col={}::{message}". This methos is preferrable
over using a problem matcher because problem matchers only allow
single-line messages, whereas ::error allows multi-line messages.

PR-URL: nodejs#34590
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@ronag @targos
stream: fix legacy pipe error handling
22bf488 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ExE-Boss @targos
doc: document changes for */promises alias modules
ef7d3b1 
Fixes: nodejs#35740

Refs: nodejs#31553
Refs: nodejs#32953
Refs: nodejs#33991
Refs: nodejs#34001
Refs: nodejs#34055
Refs: nodejs#34962

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Trivikram Kamat <trivikr.dev@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#34002
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 @targos
child_process: refactor to use more primordials
b566802 
PR-URL: nodejs#36003
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 @targos
lib,tools: enforce access to prototype from primordials
483c1a1 
PR-URL: nodejs#36025
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
timers: refactor to use more primordials
8c743ca 
PR-URL: nodejs#36132
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95 @targos
http2: add support for TypedArray to getUnpackedSettings
4d35416 
PR-URL: nodejs#36141
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 @targos
lib: refactor to use more primordials
986e79a 
PR-URL: nodejs#36140
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 @targos
errors: refactor to use more primordials
34984f3 
PR-URL: nodejs#36167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott @targos
src: fix label indentation
f4cb54d 
In preparation for a cpplint version bump, move labels to be unindented.
The one space indentation we use now is flagged in cpplint 1.5.2.

Refs: https://softwareengineering.stackexchange.com/q/151806/26510

PR-URL: nodejs#36213
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@cjihrig @targos
tools: update ESLint to 7.14.0
abecf46 
Update ESLint to 7.14.0

PR-URL: nodejs#36217
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig @targos
tools: enable no-nonoctal-decimal-escape lint rule
66597bc 
This rule is new in ESLint 7.14.0.

PR-URL: nodejs#36217
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 @targos
lib: refactor primordials.uncurryThis
73b5aa7 
This is done to avoid creating an array and gain performance.

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

PR-URL: nodejs#36221
Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@bcoe @targos
deps: V8: cherry-pick 86991d0587a1
f59ee44 
Adds methods for fetching stack trace information about
enclosing function.

Refs nodejs#36042

Original commit message:

    Reland "stack-trace-api: implement getEnclosingLine/Column"

    This reverts commit 5557a63beb5a53c93e9b590eaf2933e21bcb3768.

    Reason for revert: Sheriff's mistake, failing test was previously flaking.

    Original change's description:
    > Revert "stack-trace-api: implement getEnclosingLine/Column"
    >
    > This reverts commit c48ae2d96cbfdc2216706a5e9a79ae1dce5a638b.
    >
    > Reason for revert: Breaks a profiling test:
    > https://ci.chromium.org/p/v8/builders/ci/V8%20Win32/30010
    >
    > Original change's description:
    > > stack-trace-api: implement getEnclosingLine/Column
    > >
    > > Introduces getEnclosingColumn and getEnclosingLine on CallSite
    > > so that the position can be used to lookup the original symbol
    > > for function when source maps are used.
    > >
    > > BUG=v8:11157
    > >
    > > Change-Id: I06c4c374d172d206579abb170c7b7a2bd3bb159f
    > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2547218
    > > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    > > Commit-Queue: Benjamin Coe <bencoe@google.com>
    > > Cr-Commit-Position: refs/heads/master@{#71343}
    >
    > TBR=jkummerow@chromium.org,yangguo@chromium.org,bencoe@google.com
    >
    > Change-Id: Iab5c250c1c4fbdab86971f4a7e40abc8f87cf79c
    > No-Presubmit: true
    > No-Tree-Checks: true
    > No-Try: true
    > Bug: v8:11157
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2555384
    > Reviewed-by: Bill Budge <bbudge@chromium.org>
    > Commit-Queue: Bill Budge <bbudge@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#71345}

    TBR=bbudge@chromium.org,jkummerow@chromium.org,yangguo@chromium.org,bencoe@google.com

    # Not skipping CQ checks because this is a reland.

    Bug: v8:11157
    Change-Id: I8dba19ceb29a24594469d2cf79626f741dc4cad3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2555499
    Reviewed-by: Bill Budge <bbudge@chromium.org>
    Commit-Queue: Bill Budge <bbudge@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#71348}

Refs: v8/v8@86991d0

PR-URL: nodejs#36254
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 @targos
policy: refactor to use more primordials
ac47032 
PR-URL: nodejs#36210
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
@Lxxyx @targos
tty: refactor to use more primordials
b641c26 
PR-URL: nodejs#36272
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 @targos
os: refactor to use more primordials
1462d59 
PR-URL: nodejs#36284
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95 @targos
v8: refactor to use more primordials
d49ec9b 
PR-URL: nodejs#36285
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos and others added 4 commits May 18, 2021 08:32
@targos
deps: update ICU to 69.1
55d0cd8 
Refs: /~https://github.com/unicode-org/icu/releases/tag/release-69-1

Backport-PR-URL: nodejs#38397
PR-URL: nodejs#38178
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
deps: V8: backport 4bf051d536a1
c46dd77 
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
src: use correct outer Context’s microtask queue
0e077a5 
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
src: use correct microtask queue for checkpoints
df3d763 
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>
@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v14.x labels May 23, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented May 25, 2021 •
edited
Loading

Looks like the change depends on another PR that's not in v14.x yet:

17:00:40 ../test/cctest/test_environment.cc: In member function ‘virtual void EnvironmentTest_NestedMicrotaskQueue_Test::TestBody()’:
17:00:40 ../test/cctest/test_environment.cc:718:19: error: ‘SpinEventLoop’ was not declared in this scope
17:00:40    int exit_code = SpinEventLoop(env).FromJust();
17:00:40                    ^~~~~~~~~~~~~

Edit: it's #35597

@targos targos force-pushed the v14.x-staging branch 4 times, most recently from 49c8441 to 50b3bd0 Compare June 11, 2021 07:31
@richardlau richardlau force-pushed the v14.x-staging branch 2 times, most recently from 1f06fcf to 16dcd9c Compare July 5, 2021 16:02
@targos
Copy link
Member

targos commented Aug 8, 2021

@addaleax do you still want this to land on v14.x ? A rebase is needed and there's an issue with a test that is using an API that doesn't exist in v14.x.

@richardlau
Copy link
Member

If this is still desired for v14.x it needs a rebase and #38787 (comment) addressed.

@richardlau richardlau force-pushed the v14.x-staging branch 2 times, most recently from 30ba86c to 7f2825b Compare March 17, 2022 23:36
@targos
Copy link
Member

targos commented May 3, 2023

Closing as v14.x is EoL.

@targos targos closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.