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
+32,523 −74,735

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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
@aduh95 @targos
tools,lib: tighten prefer-primordials rules for Error statics
5068db6 
PR-URL: nodejs#36017
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
@targos
timers: cleanup abort listener on awaitable timers
538ec98 
Co-authored-by: Benjamin Gruenbaum <[email protected]>
Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#36006
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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 <[email protected]>

PR-URL: nodejs#36016
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
@aduh95 @targos
module: refactor to use more primordials
1f38c63 
PR-URL: nodejs#36024
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@cjihrig @targos
tools: update ESLint to 7.11.0
64c5f62 
Update ESLint to 7.11.0

PR-URL: nodejs#35578
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@cjihrig @targos
tools: update ESLint to 7.12.0
8b2df0c 
Update ESLint to 7.12.0

PR-URL: nodejs#35799
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@cjihrig @targos
tools: update ESLint to 7.12.1
8e4a055 
Update ESLint to 7.12.1

PR-URL: nodejs#35799
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@ronag @targos
stream: fix legacy pipe error handling
22bf488 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Co-authored-by: Trivikram Kamat <[email protected]>
Co-authored-by: Rich Trott <[email protected]>

PR-URL: nodejs#34002
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Pranshu Srivastava <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95 @targos
child_process: refactor to use more primordials
b566802 
PR-URL: nodejs#36003
Reviewed-By: Rich Trott <[email protected]>
@aduh95 @targos
lib,tools: enforce access to prototype from primordials
483c1a1 
PR-URL: nodejs#36025
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos
timers: refactor to use more primordials
8c743ca 
PR-URL: nodejs#36132
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@aduh95 @targos
http2: add support for TypedArray to getUnpackedSettings
4d35416 
PR-URL: nodejs#36141
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95 @targos
lib: refactor to use more primordials
986e79a 
PR-URL: nodejs#36140
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95 @targos
errors: refactor to use more primordials
34984f3 
PR-URL: nodejs#36167
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95 @targos
lib: refactor primordials.uncurryThis
73b5aa7 
This is done to avoid creating an array and gain performance.

Co-authored-by: ExE Boss <[email protected]>

PR-URL: nodejs#36221
Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@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 <[email protected]>
    > > Commit-Queue: Benjamin Coe <[email protected]>
    > > Cr-Commit-Position: refs/heads/master@{#71343}
    >
    > [email protected],[email protected],[email protected]
    >
    > 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 <[email protected]>
    > Commit-Queue: Bill Budge <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#71345}

    [email protected],[email protected],[email protected],[email protected]

    # 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 <[email protected]>
    Commit-Queue: Bill Budge <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71348}

Refs: v8/v8@86991d0

PR-URL: nodejs#36254
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95 @targos
policy: refactor to use more primordials
ac47032 
PR-URL: nodejs#36210
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
@Lxxyx @targos
tty: refactor to use more primordials
b641c26 
PR-URL: nodejs#36272
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95 @targos
os: refactor to use more primordials
1462d59 
PR-URL: nodejs#36284
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@aduh95 @targos
v8: refactor to use more primordials
d49ec9b 
PR-URL: nodejs#36285
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71710}

Refs: v8/v8@4bf051d

PR-URL: nodejs#36482
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@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.