src: pass resource object along with InternalMakeCallback by addaleax · Pull Request #32063 · nodejs/node

addaleax · 2020-03-03T10:33:15Z

This was an oversight in 9fdb6e6.
Fixing this is necessary to make executionAsyncResource() work
as expected.

Refs: #30959
Fixes: #32060

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060

nodejs-github-bot · 2020-03-03T10:34:21Z

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

Flarna · 2020-03-03T11:18:04Z

src/api/callback.cc

  Context::Scope context_scope(env->context());
  MaybeLocal<Value> ret =
-      InternalMakeCallback(env, recv, callback, argc, argv, asyncContext);
+      InternalMakeCallback(env, recv, recv, callback, argc, argv, asyncContext);


Not directly related to this PR but to executionAsyncResource(): Maybe we should add some ToDo comment here that we should get rid of using receiver as resource here? I think this is one of the major gaps between asyncIds (part of parameter asyncContext here) and async resources.

executionAsyncResource() wasn’t really introduced with a plan for addons/embedders in mind, I’m afraid … I’ve been thinking about how to approach this, and ultimately, it probably makes sense to include the resource as a v8::Global in the async_context struct… that might come with some overhead, but I feel like that’s the only solution that’s going to make sure that the resource + the IDs actually match?

I thought about the same.
But after I saw how simple async_context is and that this simple structure is relied on at several places (at least as far as I remember) I stopped walking deeper into it...

Add a new scenario of multiple clients sharing a single data callback function managing their response data through AsyncLocalStorage APIs Refs: nodejs#32063 Refs: nodejs#32060 Refs: nodejs#32062 (comment) Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com>

addaleax · 2020-03-04T22:14:20Z

Fast-track? Might be nice to get this into #32099. (Only red CI failure is sequential/test-timers-blocking-callback, which is causing macOS trouble elsewhere too.)

MylesBorins · 2020-03-04T22:21:09Z

Landed in 787143b

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: #30959 Fixes: #32060 PR-URL: #32063 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a new scenario of multiple clients sharing a single data callback function managing their response data through AsyncLocalStorage APIs Refs: #32063 Refs: #32060 Refs: #32062 (comment) Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #32082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060 PR-URL: nodejs#32063 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a new scenario of multiple clients sharing a single data callback function managing their response data through AsyncLocalStorage APIs Refs: nodejs#32063 Refs: nodejs#32060 Refs: nodejs#32062 (comment) Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: nodejs#32082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060 PR-URL: nodejs#32063 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a new scenario of multiple clients sharing a single data callback function managing their response data through AsyncLocalStorage APIs Refs: nodejs#32063 Refs: nodejs#32060 Refs: nodejs#32062 (comment) Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: nodejs#32082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: #30959 Fixes: #32060 PR-URL: #32063 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a new scenario of multiple clients sharing a single data callback function managing their response data through AsyncLocalStorage APIs Refs: #32063 Refs: #32060 Refs: #32062 (comment) Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #32082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

src: pass resource object along with InternalMakeCallback

b185431

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 3, 2020

addaleax mentioned this pull request Mar 3, 2020

CLS store gets reset on the first callback in http scenario #32060

Closed

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Mar 3, 2020

vdeturckheim approved these changes Mar 3, 2020

View reviewed changes

Qard approved these changes Mar 3, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 3, 2020

Flarna reviewed Mar 3, 2020

View reviewed changes

Flarna approved these changes Mar 3, 2020

View reviewed changes

gireeshpunathil mentioned this pull request Mar 3, 2020

Ability for async functions to retreive their CLS context #32062

Closed

gireeshpunathil approved these changes Mar 3, 2020

View reviewed changes

jasnell approved these changes Mar 3, 2020

View reviewed changes

HarshithaKP mentioned this pull request Mar 4, 2020

test: add new scenario for async-local storage #32082

Closed

4 tasks

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 4, 2020

addaleax mentioned this pull request Mar 4, 2020

v13.10.1 proposal #32099

Merged

MylesBorins closed this Mar 4, 2020

targos added backported-to-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 7, 2020

puzpuzpuz mentioned this pull request Mar 7, 2020

[v12.x backport] async_hooks: add executionAsyncResource #32131

Closed

4 tasks

Flarna mentioned this pull request Apr 9, 2020

async_hooks performance/stability improvement plans nodejs/diagnostics#376

Closed

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: pass resource object along with InternalMakeCallback#32063

src: pass resource object along with InternalMakeCallback#32063
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:exec-async-resource-internal-makecallback

addaleax commented Mar 3, 2020

Uh oh!

nodejs-github-bot commented Mar 3, 2020

Uh oh!

Flarna Mar 3, 2020

Uh oh!

addaleax Mar 3, 2020

Uh oh!

Flarna Mar 3, 2020

Uh oh!

addaleax commented Mar 4, 2020

Uh oh!

MylesBorins commented Mar 4, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

addaleax commented Mar 3, 2020

Checklist

Uh oh!

nodejs-github-bot commented Mar 3, 2020

Uh oh!

Flarna Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

Flarna Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 4, 2020

Uh oh!

MylesBorins commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

MylesBorins commented Mar 4, 2020 •

edited

Loading