镜像站点 · 本页由第三方 GitHub 只读镜像提供，非 GitHub 官方站点，不接受任何登录或凭据输入。前往 github.com
Skip to content

allow retrieving of node::async_context from Nan::AsyncResource - #1026

Open
mmomtchev wants to merge 5 commits into
nodejs:mainfrom
mmomtchev:makeCallbackScope
Open

allow retrieving of node::async_context from Nan::AsyncResource#1026
mmomtchev wants to merge 5 commits into
nodejs:mainfrom
mmomtchev:makeCallbackScope

Conversation

@mmomtchev

@mmomtchev mmomtchev commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refs: #1024

Allow retrieving the node::async_context from Nan::AsyncResrouce in order to construct node::CallbackScope.

The problem is that the node::async_context is a private member of nan::AsyncResource.

This allows to run properly the microtasks queue for async workers that do not call JavaScript.

This require Node.js 9.0.0, as before this version the microtasks queue was always run automatically.

In reality, Node.js 26.4.0 is the first version that actually does not do this, although this has been specified since Node.js 9.0.0.

mmomtchev
mmomtchev commented Sep 23, 2026
View reviewed changes

@mmomtchev mmomtchev left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Returning a non-copyable object from a factory using an initializer-list is C++11.

Flarna
Flarna reviewed Sep 25, 2026
View reviewed changes
Comment thread nan.h
inline node::CallbackScope makeCallbackScope(v8::Local<v8::Object> target) {
return {v8::Isolate::GetCurrent(), target, context};
}
#endif

@Flarna Flarna Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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 ifdef for all users supporting ancient node.js versions if there is no dummy implementation for these older node.js versions?

@mmomtchev mmomtchev Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is an #ifdef.

@mmomtchev mmomtchev Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

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.

@kkoopa kkoopa Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But here AsyncResource does 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.

@kkoopa kkoopa Oct 15, 2026

Copy link
Copy Markdown
Collaborator

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.

@mmomtchev mmomtchev Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

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::CallbackScope does not exist, so the user will have to use an ifdef.

The only way that I see around this is to wrap the whole construction and add a Nan::CallbackScope that is an empty object on Node <9.0.0.

@kkoopa kkoopa Oct 15, 2026

Copy link
Copy Markdown
Collaborator

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::CallbackScope is fine, but would it not make sense to have it just do a Nan::MakeCallback for 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?

@mmomtchev mmomtchev Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nan::CallbackScope is 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.

@mmomtchev mmomtchev Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

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 #error in 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 Promise was common and all C++ modules used callbacks which do not have this problem.

@kkoopa

kkoopa commented Sep 25, 2026 via email

Copy link
Copy Markdown
Collaborator

@kkoopa

kkoopa commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Alright, I got back to this now. I like what I see. Based on what I could see from the discussions over in nodejs/node, it seems like doing this is the way forward. Using the C++11 feature for NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION is fine, since that Node/V8 pairing already requires C++11. I would just like an ifdef returning a noop dummy object for older versions and since node::CallbackScope itself might also be fairly new (I know..., it is ancient) and not exist everywhere, it probably needs wrapping inside a Nan::CallbackScope (which would be a dummy object on ancient versioins).

@kkoopa

kkoopa commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

The problem with calling node::CallbackScope directly is that the node::async_context is a private member of nan::AsyncResource.

I might lack a bit of context here, but could this not be resolved with a friend function?

@mmomtchev

mmomtchev commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

The problem with calling node::CallbackScope directly is that the node::async_context is a private member of nan::AsyncResource.

I might lack a bit of context here, but could this not be resolved with a friend function?

No, the user class needs it.

@kkoopa

kkoopa commented Oct 14, 2026 via email

Copy link
Copy Markdown
Collaborator

@mmomtchev

mmomtchev commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

In fact, when looking at node-addon-api where the situation is similar, they went the other way around, with an operator node::async_context instead of a factory and I think that this is indeed less awkward for the user. I will change it, but I am just waiting to see where this is going because there will be an outcry from async module authors when people start switching to Node 26. Most people still do not realise the implications. I think there should be at least a "heads up" from Node.

@kkoopa

kkoopa commented Oct 15, 2026 via email

Copy link
Copy Markdown
Collaborator

@mmomtchev mmomtchev changed the title add AsyncResource::makeCallbackScope allow retrieving of node::async_context from Nan::AsyncResource Oct 15, 2026
@mmomtchev

mmomtchev commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

There was one segfault on a different test on macos-latest with Node 26. It does not seem to be easily reproducible.

@kkoopa

kkoopa commented Oct 15, 2026 via email

Copy link
Copy Markdown
Collaborator

@mmomtchev

mmomtchev commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

The single entry point API for handling the micro tasks was actually introduced in Node 8:
https://github.com/nodejs/node/pull/14697/changes

In Node 6, calling the micro tasks was implemented directly in MakeCallback:
nodejs/node@41f333e

Before this it was somewhat of a mess that was spread around, including the http module. There was a KickNextTick entry point.

I am afraid that there is no safe way to implement this feature - running the micro tasks without JS on the stack and without calling JS - below Node 8. At the very least, it will require extensive testing with older Node versions.

@kkoopa

kkoopa commented Oct 15, 2026 via email

Copy link
Copy Markdown
Collaborator

@mmomtchev

mmomtchev commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Maybe it should fail with an #error on older Node versions.

@kkoopa

kkoopa commented Oct 16, 2026 via email

Copy link
Copy Markdown
Collaborator

@kkoopa

kkoopa commented Oct 16, 2026 via email

Copy link
Copy Markdown
Collaborator

@kkoopa

kkoopa commented Oct 16, 2026 via email

Copy link
Copy Markdown
Collaborator

@mmomtchev

mmomtchev commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Why not just an #error with #ifdef?

@mmomtchev

mmomtchev commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

In fact, if the goal is to emit an error only if the code is actually used, the best way is to simply omit the function when it is not there. This way code that uses it won't compile on older Node versions.

@mmomtchev

mmomtchev commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

This will print the line:

#if NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION
  inline operator node::async_context() const {
    return context;
  }
#else
  inline operator node::async_context() = delete; // Not supported before Node 9
#endif

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Default deleted constructors are C++11, sadly only acceptable for Node 4 and later, otherwise elegant. I did a slightly more fleshed out version of the SFINAE thing, not sure if it can be made finer than the whole AsyncResource class, but then again AsyncResource would not really work on those ancient versions anyway:

#include <cstdio>

namespace imp {
	template<bool E>
	struct Assertion{
		char assert[2 * static_cast<int>(E) - 1];
	};

	template<bool E>
	class AsyncResource{
		struct Assertion<E> assertion;
	};
};

typedef imp::AsyncResource<NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION> AsyncResource;

int main() {
	AsyncResource b;
	printf("OK\n");
}

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

The goal would be to have code using the next release of NAN still compile on ancient versions, as long as AsyncResource or, more specifically, this particular feature of AsyncResource is not used in user code. If it is used on an ancient version, any kind of compilation error is acceptable. That seems like the least messy way forward.

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Okay, did some further testing. Just not having the explicit delete there gives a nice error about the conversion operator not existing, which is good enough. So, I think this should suffice:

#include <cstdio>

/* node.h */
namespace node {
#if NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION
	struct async_context {
		int m;
	};
#endif
};

/* nan.h */
/* ugly hack to node namespace making it compilable */
#if NODE_MODULE_VERSION < NODE_9_0_MODULE_VERSION
namespace node {
	struct async_context {
	};
};
#endif

namespace Nan {
	struct AsyncResource {
#if NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION
		inline operator node::async_context() const {
			return context;
		}
#elif NODE_MODULE_VERSION >= NODE_4_0_MODULE_VERSION
		inline operator node::async_context() = delete; // Not supported before Node 9
#endif

#if NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION
		node::async_context context;
#endif
	};
};

/* app.cc */
int main() {
	Nan::AsyncResource b;
	b.operator node::async_context(); /* gives compilation error on node < 9 */
	printf("OK\n");
}

Seems to do what it should with all permutations of -DNODE_MODULE_VERSION and -std in cc -std=c++03 -DNODE_4_0_MODULE_VERSION=4 -DNODE_9_0_MODULE_VERSION=9 -DNODE_MODULE_VERSION=3 sfinae_op.cc && ./a.out

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Actually, the branch for NODE_MODULE_VERSION >= 4 should better be a test for CXX11.

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Or just leave said branch out to avoid the ugly hack in node namespace, that is probably nicer.

@mmomtchev

mmomtchev commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

If you simply omit the function with an #ifdef on older versions, you will still get a compile error only when use it on older Node versions. The template version won't allow you to display a meaningful error message - in fact I think that there are more chances to have a very confusing message with it.

@kkoopa

kkoopa commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

I agree, that is what I concluded in my last message. So it would end up like this:

namespace Nan {
	class AsyncResource {
#if NODE_MODULE_VERSION >= NODE_9_0_MODULE_VERSION
		inline operator node::async_context() const {
			return context;
		}
#endif
	};
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkoopa kkoopa kkoopa left review comments
+1 more reviewer
@Flarna Flarna Flarna left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mmomtchev @kkoopa @Flarna