src: pass resource object along with InternalMakeCallback #32063
+42
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060
Collaborator
vdeturckheim
approved these changes
Mar 3, 2020
Qard
approved these changes
Mar 3, 2020
Flarna
reviewed
Mar 3, 2020
| Context::Scope context_scope(env->context()); | ||
| MaybeLocal<Value> ret = | ||
| InternalMakeCallback(env, recv, callback, argc, argv, asyncContext); | ||
| InternalMakeCallback(env, recv, recv, callback, argc, argv, asyncContext); |
Member
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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?
Member
There was a problem hiding this comment.
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...
Flarna
approved these changes
Mar 3, 2020
gireeshpunathil
approved these changes
Mar 3, 2020
jasnell
approved these changes
Mar 3, 2020
HarshithaKP
added a commit
to HarshithaKP/node
that referenced
this pull request
Mar 4, 2020
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>
4 tasks
Member
Author
|
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.) |
Merged
Contributor
|
Landed in 787143b |
MylesBorins
pushed a commit
that referenced
this pull request
Mar 4, 2020
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>
MylesBorins
pushed a commit
that referenced
this pull request
Mar 4, 2020
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>
4 tasks
addaleax
pushed a commit
that referenced
this pull request
Mar 13, 2020
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>
BridgeAR
pushed a commit
that referenced
this pull request
Mar 17, 2020
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>
MylesBorins
pushed a commit
that referenced
this pull request
Mar 24, 2020
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>
puzpuzpuz
pushed a commit
to puzpuzpuz/node
that referenced
this pull request
Apr 14, 2020
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>
puzpuzpuz
pushed a commit
to puzpuzpuz/node
that referenced
this pull request
Apr 14, 2020
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>
puzpuzpuz
pushed a commit
to puzpuzpuz/node
that referenced
this pull request
Apr 14, 2020
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>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was an oversight in 9fdb6e6.
Fixing this is necessary to make
executionAsyncResource()workas expected.
Refs: #30959
Fixes: #32060
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes