Repository navigation
allow retrieving of node::async_context from Nan::AsyncResource
#1026
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
73b3f76
add AsyncResource::makeCallbackScope
mmomtchev 2204f25
replace the awkward factory method with an operator to match the node…
mmomtchev 234b300
do not ignore the returned value of Resolver::Resolve()
mmomtchev 460e554
remove the superfluous typecast
mmomtchev f9237f6
replace the C-style cast with static_cast
mmomtchev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| /********************************************************************* | ||
| * NAN - Native Abstractions for Node.js | ||
| * | ||
| * Copyright (c) 2018 NAN contributors | ||
| * | ||
| * MIT License <https://github.com/nodejs/nan/blob/master/LICENSE.md> | ||
| ********************************************************************/ | ||
|
|
||
| #include <nan.h> | ||
|
|
||
| #include "sleep.h" // NOLINT(build/include_subdir) | ||
|
|
||
| using namespace Nan; // NOLINT(build/namespaces) | ||
|
|
||
| class DelayRequest : public AsyncResource { | ||
| public: | ||
| DelayRequest(int milliseconds_, v8::Local<v8::Promise::Resolver> resolver_) | ||
| : AsyncResource("nan:test.DelayPromise"), | ||
| milliseconds(milliseconds_) { | ||
| resolver.Reset(resolver_); | ||
| request.data = this; | ||
| } | ||
| ~DelayRequest() { | ||
| resolver.Reset(); | ||
| } | ||
|
|
||
| Persistent<v8::Promise::Resolver> resolver; | ||
| uv_work_t request; | ||
| int milliseconds; | ||
| }; | ||
|
|
||
| void Delay(uv_work_t* req) { | ||
| DelayRequest *delay_request = static_cast<DelayRequest*>(req->data); | ||
| Sleep(delay_request->milliseconds); | ||
| } | ||
|
|
||
| void AfterDelay(uv_work_t* req, int status) { | ||
| HandleScope scope; | ||
|
|
||
| DelayRequest *delay_request = static_cast<DelayRequest*>(req->data); | ||
| v8::Local<v8::Promise::Resolver> resolver = New(delay_request->resolver); | ||
| resolver->Resolve(Nan::GetCurrentContext(), New<v8::Boolean>(true)).ToChecked(); | ||
|
|
||
| v8::Local<v8::Object> target = New<v8::Object>(); | ||
|
|
||
| // Run the callback in the async context. | ||
| node::CallbackScope callback_scope(v8::Isolate::GetCurrent(), target, | ||
| (node::async_context)*delay_request); | ||
|
|
||
| delete delay_request; | ||
| } | ||
|
|
||
| NAN_METHOD(Delay) { | ||
| int delay = To<int>(info[0]).FromJust(); | ||
| v8::Local<v8::Promise::Resolver> resolver = | ||
| v8::Promise::Resolver::New(Nan::GetCurrentContext()).ToLocalChecked(); | ||
|
|
||
| DelayRequest* delay_request = new DelayRequest(delay, resolver); | ||
|
|
||
| info.GetReturnValue().Set(resolver->GetPromise()); | ||
|
|
||
| uv_queue_work( | ||
| GetCurrentEventLoop() | ||
| , &delay_request->request | ||
| , Delay | ||
| , reinterpret_cast<uv_after_work_cb>(AfterDelay)); | ||
| } | ||
|
|
||
| NAN_MODULE_INIT(Init) { | ||
| Set(target, New<v8::String>("delay").ToLocalChecked(), | ||
| GetFunction(New<v8::FunctionTemplate>(Delay)).ToLocalChecked()); | ||
| } | ||
|
|
||
| NODE_MODULE(asyncresource_promise, Init) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /********************************************************************* | ||
| * NAN - Native Abstractions for Node.js | ||
| * | ||
| * Copyright (c) 2026 NAN contributors | ||
| * | ||
| * MIT License <https://github.com/nodejs/nan/blob/master/LICENSE.md> | ||
| ********************************************************************/ | ||
|
|
||
| const version = process.versions.node.split('.'); | ||
| if (version[0] < 9) { | ||
| process.exit(0); | ||
| } | ||
|
|
||
| try { | ||
| require('async_hooks'); | ||
| } catch (e) { | ||
| process.exit(0); | ||
| } | ||
|
|
||
| const test = require('tap').test | ||
| , testRoot = require('path').resolve(__dirname, '..') | ||
| , delay = require('bindings')({ module_root: testRoot, bindings: 'asyncresource-promise' }).delay | ||
| , asyncHooks = require('async_hooks'); | ||
|
|
||
| test('asyncresource-promise', function (t) { | ||
| t.plan(2); | ||
|
|
||
| var beforeCalled = false; | ||
| var promiseResolveCalled = false; | ||
|
|
||
| var hooks = asyncHooks.createHook({ | ||
| init: function (asyncId, type, triggerAsyncId, resource) { | ||
| if (type === 'nan:test.DelayPromise') { | ||
| resourceAsyncId = asyncId; | ||
| } | ||
| }, | ||
| before: function (asyncId) { | ||
| if (asyncId === resourceAsyncId) { | ||
| beforeCalled = true; | ||
| } | ||
| }, | ||
| promiseResolve: function (asyncId) { | ||
| promiseResolveCalled = true; | ||
| } | ||
|
|
||
| }); | ||
| hooks.enable(); | ||
|
|
||
| originalExecutionAsyncId = asyncHooks.executionAsyncId(); | ||
| delay(1000).then(function() { | ||
| t.ok(beforeCalled, 'before should have been called'); | ||
| t.ok(promiseResolveCalled, 'promiseResolve should have been called'); | ||
| }) | ||
| }); |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this require an
ifdeffor all users supporting ancient node.js versions if there is no dummy implementation for these older node.js versions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an
#ifdef.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it comes to the users, the users do not have any choice as this does not work the same way before Node.js 9.0.0. There are no
AsyncResources at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here
AsyncResourcedoes exist for all versions of Node, it just does not perform the non-applicable operations on old versions, leading to the same observable end result.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why is the typecast needed now?
::context is a node::async_context.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that
node::CallbackScopedoes not exist, so the user will have to use anifdef.The only way that I see around this is to wrap the whole construction and add a
Nan::CallbackScopethat is an empty object on Node <9.0.0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I understand. Introducing a dummy
Nan::CallbackScopeis fine, but would it not make sense to have it just do aNan::MakeCallbackfor old versions, so the function at least gets called. Am I right in thinking that on versions of Node before 9.0, the closest match would be to just run the callback and call it a day?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nan::CallbackScopeis just a RIAA guard that means to run the micro tasks when the scope is exited. I wonder how did this work before Node 9.0.0 which introduced the async contexts, 'CallbackScopeandMakeCallback`.I am not very sure if before Node 9, binary modules had to run themselves the micro tasks or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the proposal is to add an
#errorin this case.The rationale is that this is something modules will have to add now in order to work correctly on Node 26+, while dropping support for Node 8 and earlier.
Anyway, I am afraid that there is no way to safely implement this on Node 8 - this was before
Promisewas common and all C++ modules used callbacks which do not have this problem.