src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

addaleax · 2019-11-03T12:37:56Z

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.

addaleax · 2019-11-03T12:42:56Z

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/

src/node_internals.h

nodejs-github-bot · 2019-11-04T07:39:57Z

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

nodejs-github-bot · 2019-11-04T09:09:05Z

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

nodejs-github-bot · 2019-11-04T12:52:49Z

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

nodejs-github-bot · 2019-11-05T20:18:07Z

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

nodejs-github-bot · 2019-11-05T22:03:03Z

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

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>

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 · 2019-11-06T13:06:59Z

Landed in d80e49d 4aca277

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>

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>

targos · 2019-12-01T09:44:24Z

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

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>

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>

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>

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>

nodejs/node#30236

Refs: nodejs@4aca277 Refs: nodejs#30236 Fixes: nodejs#31796

nodejs/node#30236

MylesBorins · 2020-02-18T01:35:45Z

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 · 2020-02-18T01:45:26Z

@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.

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 · 2020-02-18T02:03:03Z

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

nodejs/node#30236

* 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>

Refs: 4aca277 Refs: #30236 Fixes: #31796 PR-URL: #31801 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

addaleax added 2 commits November 3, 2019 13:01

src: use callback scope for main script

cd8e3a6

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

src: remove AsyncScope and AsyncCallbackScope

3f557fc

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

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

fhinkel approved these changes Nov 3, 2019

View reviewed changes

devsnek approved these changes Nov 3, 2019

View reviewed changes

devnexen reviewed Nov 4, 2019

View reviewed changes

src/node_internals.h Show resolved Hide resolved

devnexen approved these changes Nov 4, 2019

View reviewed changes

fixup! src: remove AsyncScope and AsyncCallbackScope

dc88e20

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

fixup! fixup! src: remove AsyncScope and AsyncCallbackScope

a299576

jasnell approved these changes Nov 6, 2019

View reviewed changes

addaleax closed this Nov 6, 2019

addaleax deleted the main-script-cb-scope branch November 6, 2019 13:07

BridgeAR mentioned this pull request Nov 19, 2019

v13.2.0 proposal #30547

Merged

targos added the backport-blocked-v12.x label Dec 1, 2019

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020

Fixup InternalCallbackScope patch

7da7ba2

nodejs/node#30236

addaleax added a commit to addaleax/node that referenced this pull request Feb 14, 2020

src: inform callback scopes about exceptions in HTTP parser

a8ab805

Refs: nodejs@4aca277 Refs: nodejs#30236 Fixes: nodejs#31796

addaleax mentioned this pull request Feb 14, 2020

src: inform callback scopes about exceptions in HTTP parser #31801

Closed

3 tasks

codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020

Fixup InternalCallbackScope patch

c940be3

nodejs/node#30236

MylesBorins added backport-requested-v12.x and removed backport-blocked-v12.x labels Feb 18, 2020

addaleax added a commit that referenced this pull request Feb 18, 2020

src: inform callback scopes about exceptions in HTTP parser

fab3eff

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 removed the backport-requested-v12.x label Feb 18, 2020

targos mentioned this pull request Feb 18, 2020

v12.16.0 post-mortem nodejs/Release#536

Closed

codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020

Fixup InternalCallbackScope patch

e5048d5

nodejs/node#30236

codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020

Fixup InternalCallbackScope patch

7b524b7

nodejs/node#30236

codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020

Fixup InternalCallbackScope patch

e1469ef

nodejs/node#30236

codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020

Fixup InternalCallbackScope patch

01444e0

nodejs/node#30236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

addaleax commented Nov 3, 2019

addaleax commented Nov 3, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

addaleax commented Nov 6, 2019

targos commented Dec 1, 2019

MylesBorins commented Feb 18, 2020

addaleax commented Feb 18, 2020

MylesBorins commented Feb 18, 2020

src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

Conversation

addaleax commented Nov 3, 2019

src: use callback scope for main script

src: remove AsyncScope and AsyncCallbackScope

Checklist

addaleax commented Nov 3, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 4, 2019

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

addaleax commented Nov 6, 2019

targos commented Dec 1, 2019

MylesBorins commented Feb 18, 2020

addaleax commented Feb 18, 2020

MylesBorins commented Feb 18, 2020