search

nodejs / nan

Native Abstractions for Node.js
MIT License
3.28k stars 502 forks source link

async_hooks in node 8 #739

Open kkoopa opened 6 years ago

kkoopa commented 6 years ago

There were major changes to async_hooks between 8.5.0 and 8.6.0. The same setup that works on 8.6.0 and above results in the following assertion on 8.5.0: ../src/node.cc:1379:v8::MaybeLocal<v8::Value> node::MakeCallback(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context): Assertion '(env->current_async_id()) == (asyncContext.async_id)' failed.

Until this is resolved, async_hooks will remain disabled for all of Node 8. See #738.

bnoordhuis commented 6 years ago

cc @ofrobots

kkoopa commented 6 years ago

After closer inspection, it seems the problem stems from env->current_async_id() always returning 0.

ofrobots commented 6 years ago

Thanks for the ping @bnoordhuis. I have sporadic time availability due to travel presently – I will take a look when possible.

Flarna commented 6 years ago

Not sure if this is the root cause of the above assert but in https://github.com/nodejs/node/pull/14040 there was a significant change in async hooks API but it was not applied on 8.6.0 (already before) e.g. node::EmitAsyncInit() requires 4 parameters in Node 8.0.0 but only 3 mandatory in latest 8.x. Also return type differs.

Above PR introduced also a backward compatible APIs. Maybe using this for 8.x allows to introduce async_hooks in NAN already for 8.x and still allow reuse of binaries for all 8.x versions.

kkoopa commented 6 years ago

Not sure if this is the root cause of the above assert but in nodejs/node#14040 there was a significant change in async hooks API but it was not applied on 8.6.0 (already before) e.g. node::EmitAsyncInit() requires 4 parameters in Node 8.0.0 but only 3 mandatory in latest 8.x. Also return type differs.

Yes, that partly applies to Node 8.0.0, but is rather easily worked around. I have code which compiles with 8.0.0, but still does not have an execution scope set up and leads to the assert triggering.

It seems (almost) all minor versions between 8.0 and 8.5 will need testing and some shimming and conditional compilation to smooth out differences. The javascript API is also affected by changes and renaming, but is out of scope for NAN, apart from getting the tests running correctly.

Above PR introduced also a backward compatible APIs. Maybe using this for 8.x allows to introduce async_hooks in NAN already for 8.x and still allow reuse of binaries for all 8.x versions.

This seems interesting. I had not thought about using the legacy API in this way, but it might actually work. It might lead to some deprecation warnings, but then again those old versions of Node 8 should not be used anyway, so it could be worth paying that price.