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

src: pass resource object along with InternalMakeCallback #32063

Closed
wants to merge 1 commit into from
Closed

src: pass resource object along with InternalMakeCallback #32063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/api/callback.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ void InternalCallbackScope::Close() {
}

MaybeLocal<Value> InternalMakeCallback(Environment* env,
Local<Object> resource,
Local<Object> recv,
const Local<Function> callback,
int argc,
Expand All @@ -150,7 +151,7 @@ MaybeLocal<Value> InternalMakeCallback(Environment* env,
CHECK(!argv[i].IsEmpty());
#endif

InternalCallbackScope scope(env, recv, asyncContext);
InternalCallbackScope scope(env, resource, asyncContext);
if (scope.Failed()) {
return MaybeLocal<Value>();
}
Expand Down Expand Up @@ -224,7 +225,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
CHECK_NOT_NULL(env);
Context::Scope context_scope(env->context());
MaybeLocal<Value> ret =
InternalMakeCallback(env, recv, callback, argc, argv, asyncContext);
InternalMakeCallback(env, recv, recv, callback, argc, argv, asyncContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (ret.IsEmpty() && env->async_callback_scope_depth() == 0) {
// This is only for legacy compatibility and we may want to look into
// removing/adjusting it.
Expand Down
2 changes: 1 addition & 1 deletion src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
ProviderType provider = provider_type();
async_context context { get_async_id(), get_trigger_async_id() };
MaybeLocal<Value> ret = InternalMakeCallback(
env(), object(), cb, argc, argv, context);
env(), GetResource(), object(), cb, argc, argv, context);

// This is a static call with cached values because the `this` object may
// no longer be alive at this point.
Expand Down
1 change: 1 addition & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ static v8::MaybeLocal<v8::Object> New(Environment* env,

v8::MaybeLocal<v8::Value> InternalMakeCallback(
Environment* env,
v8::Local<v8::Object> resource,
v8::Local<v8::Object> recv,
const v8::Local<v8::Function> callback,
int argc,
Expand Down
37 changes: 37 additions & 0 deletions test/async-hooks/test-async-exec-resource-http-32060.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';
require('../common');
const assert = require('assert');
const {
executionAsyncResource,
executionAsyncId,
createHook,
} = require('async_hooks');
const http = require('http');

const hooked = {};
createHook({
init(asyncId, type, triggerAsyncId, resource) {
hooked[asyncId] = resource;
}
}).enable();

const server = http.createServer((req, res) => {
res.write('hello');
setTimeout(() => {
res.end(' world!');
}, 1000);
});

server.listen(0, () => {
assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
http.get({ port: server.address().port }, (res) => {
assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
res.on('data', () => {
assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
});
res.on('end', () => {
assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
server.close();
});
});
});