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: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 3, 2019

src: use callback scope for main script

This allows removing custom code for setting the current async ids
and running nextTicks.

src: remove AsyncScope and AsyncCallbackScope

Reduce the number of different scopes we use for async callbacks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This allows removing custom code for setting the current async ids
and running nextTicks.
Reduce the number of different scopes we use for async callbacks.
@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 Nov 3, 2019
@addaleax
Copy link
Member Author

addaleax commented Nov 3, 2019

CI is broken right now, but here’s a benchmark CI run to make sure the StreamPipe code changes aren’t affecting performance: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/457/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Nov 6, 2019
This allows removing custom code for setting the current async ids
and running nextTicks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Nov 6, 2019
Reduce the number of different scopes we use for async callbacks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Nov 6, 2019

Landed in d80e49d 4aca277

@addaleax addaleax closed this Nov 6, 2019
@addaleax addaleax deleted the main-script-cb-scope branch November 6, 2019 13:07
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
This allows removing custom code for setting the current async ids
and running nextTicks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
Reduce the number of different scopes we use for async callbacks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
@targos
Copy link
Member

targos commented Dec 1, 2019

To land on v12.x, this should either wait for #29874, or be backported.

MylesBorins pushed a commit that referenced this pull request Jan 10, 2020
This allows removing custom code for setting the current async ids
and running nextTicks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2020
Reduce the number of different scopes we use for async callbacks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This allows removing custom code for setting the current async ids
and running nextTicks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Reduce the number of different scopes we use for async callbacks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
addaleax added a commit to addaleax/node that referenced this pull request Feb 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
@MylesBorins
Copy link
Contributor

I've reverted this from the 12.x release branch as there were regressions found in #31796

This will require #31801 to fix the bug, this should be manually backported to include that commit once it has gone out in a 13.x release

/cc @addaleax

@addaleax
Copy link
Member Author

@MylesBorins I do think that our policy is, frankly, bad, if we prefer reverts on select branches to applying straightforward fixes. I’ll open an issue in nodejs/release, I think we should fix this sooner rather than later.

For this particular PR, only the second commit here is problematic. You can feel free to remove the revert of the former.

Also, it looks like you reverted on the staging branch, not the release branch. I assume that’s what you intended to do, but just in case it’s not, I thought I’d let you know.

addaleax added a commit that referenced this pull request Feb 18, 2020
Refs: 4aca277
Refs: #30236
Fixes: #31796

PR-URL: #31801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2020
Refs: 4aca277
Refs: #30236
Fixes: #31796

PR-URL: #31801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@MylesBorins
Copy link
Contributor

As the fix is straightforward and has landed on master I've backported to 12.x and rebased out the revert.

codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0

* Fixup asar support setup patch

nodejs/node#30862

* Fixup InternalCallbackScope patch

nodejs/node#30236

* Fixup GN buildfiles patch

nodejs/node#30755

* Fixup low-level hooks patch

nodejs/node#30466

* Fixup globals require patch

nodejs/node#31643

* Fixup process stream patch

nodejs/node#30862

* Fixup js2c modification patch

nodejs/node#30755

* Fixup internal fs override patch

nodejs/node#30610

* Fixup context-aware warn patch

nodejs/node#30336

* Fixup Node.js with ltcg config

nodejs/node#29388

* Fixup oaepLabel patch

nodejs/node#30917

* Remove redundant ESM test patch

nodejs/node#30997

* Remove redundant cli flag patch

nodejs/node#30466

* Update filenames.json

* Remove macro generation in GN build files

nodejs/node#30755

* Fix some compilation errors upstream

* Add uvwasi to deps

nodejs/node#30258

* Fix BoringSSL incompatibilities

* Fixup linked module patch

nodejs/node#30274

* Add missing sources to GN uv build

libuv/libuv#2347

* Patch some uvwasi incompatibilities

* chore: bump Node.js to v12.6.1

* Remove mark_arraybuffer_as_untransferable.patch

nodejs/node#30549

* Fix uvwasi build failure on win

* Fixup --perf-prof cli option error

* Fixup early cjs module loading

* fix: initialize diagnostics properly

nodejs/node#30025

* Disable new esm syntax specs

nodejs/node#30219

* Fixup v8 weakref hook spec

nodejs/node#29874

* Fix async context timer issue

* Disable monkey-patch-main spec

It relies on nodejs/node#29777, and we don't
override prepareStackTrace.

* Disable new tls specs

nodejs/node#23188

We don't support much of TLS owing to schisms between BoringSSL and
OpenSSL.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 27, 2020
Refs: 4aca277
Refs: #30236
Fixes: #31796

PR-URL: #31801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants