MakeCallback is dangerous, not only or best way of calling callback; provide direct method

[metamatt]

NodeJS lets you invoke a "callback" (really, invoke JS code from C++ code) 2 ways:

• calling the callback directly; if you have a Handle<Function> foo you can just foo->Call()
• calling the callback via MakeCallback (in src/node.cc)

The former ("direct") case just calls from C++ into JS synchronously, and returns back into C++ when the JS code returns.

The latter (MakeCallback) case calls the JS callback function, but then also ends the current tick, calling process._tickCallback() and any other callbacks registered via nextTick(). This form is intended only for use by C++ code that was dispatched from the libuv event loop, and not for C++ code that was invoked from JS code. Because in that case (JS code "A" running, calls into C++ code in a node addon, the addon's C++ code calls MakeCallback, MakeCallback dispatches the nextTick callbacks) the JS code marked "A" has been preempted, and state changed by the nextTick callbacks can be corrupted. See TooTallNate/node-weak#35 for an example of how dangerous this is.

Use cases for node addons (that would want to use nan) wanting to invoke JS code from C++ code that was invoked directly from JS (not from a libuv event callback) include

• node-weak (again, node-weak causes JS code preemption due to MakeCallback TooTallNate/node-weak#35 is a practical example of this)
• if you want to use V8 interceptors to provide Proxy-like abilities for a dynamic object to trap accesses and call JS handler routines

For these cases, use of MakeCallback is not only incorrect but extremely dangerous. The right answer is to call the callback directly. But nan doesn't expose a way of doing this; NanCallback::Call is actually wrapping MakeCallback. So is the right answer to avoid NanCallback altogether? I don't think so; it serves a very real need to deal more easily with V8's memory management; using Persistent<Function> got harder in newer V8 (node 0.11/0.12) and so it's much nicer to use NanCallback instead of raw Handle<Function>. So I think NanCallback should provide separate methods for calling a callback directly and calling a callback via MakeCallback, and make the difference really obvious so people don't accidentally do the latter. (Note in my patch for the node-weak instance of this, I do use NanCallback for memory management, but then have to go to some lengths to extract the real Handle<Function> to call directly.)

I think the problem here is exacerbated by (a) nebulous terminology like "callback" (is any function that gets called a "callback"? Not all callbacks are the same) and (b) the difficulty of using Persistent<Function> without NanCallback. But when you do use NanCallback and because of (a), it's just too tempting to accidentally call NanCallback::Call and thus MakeCallback and then preempt your JS code and then suffer all manner of heisenbugs.

So my suggestion and request is:

• add a new NanCallback::CallDirect that wraps Handle<Function>::Call
• rename NanCallback::Call to NanCallback::CallAndEndTick (or something similarly explicit that's difficult to confuse)
• in a perfect world, remove NanCallback::Call. That will probably be too annoying to addon authors already using it correctly. Then, at least deprecate it (have it generate a compile-time warning, and then act as CallAndEndTick?)

[kkoopa]

Hmm, this is actually the first time I've heard of this problem. My experience is completely opposite in that not using node::MakeCallback as of 0.11 would lead to a whole bunch of strange errors exactly because process._tickCallback() was not called and the same goes for domain support. Normally, every call would want to be done through node::MakeCallback for this reason.

Might introduce NanCallback:: CallDirect for convenience, but renaming or deprecating NanCallback::Call seems less useful. The documentation could however be improved.

[metamatt]

See also nodejs/node-v0.x-archive#9245.

[metamatt]

@kkoopa, can you elaborate on the "whole bunch of strange errors"?

Barring evidence to the contrary, I'm pretty adamant that inviting people to use MakeCallback over-eagerly is a really bad idea. See https://gist.github.com/metamatt/4ce96adbf14de9a97d41, https://gist.github.com/metamatt/99ef2a5e12de2acd5e39. I don't know anything about (haven't tried to use) domain support, so maybe I'm missing something there. But if you call MakeCallback in the (I would assume fairly standard/common) case where your addon C++ code was invoked from JS in the same domain, you can preempt the JS code that invoked you and break NodeJS's entire threadless/consistency/concurrency model.

I don't mean to sound too harsh here. But this is not a theoretical concern; this was what actually happened with TooTallNate/node-weak#35 and it caused all manner of heisenbugs in every part of our system that were extremely difficult to track down, and were all caused by this one thing (and all went away when I patched node-weak to not do that).

[kkoopa]

In some situations, not using MakeCallback would lead to strange errors such as the next tick never happening, causing all manners of normal code to fail. Now I don't know if this only applied to code invoked from libuv. I do believe this is a genuine concern, but want the common case adjusted for what is normal. It's not exactly nice to have to know, and keep track of, using one way of calling in certain places and other ways in other sections. What I would like is one single way of calling a JS function from C++ that always works, no matter what.

[metamatt]

Let's see if we get any traction on nodejs/node-v0.x-archive#9245 then... in TooTallNate/node-weak#36 (comment) @trevnorris said that MakeCallback should be safe for use (modulo a small performance hit), or at least that's the intent. I observe otherwise today, but if that becomes true, then we should have your "one single way that always works".

[kkoopa]

Yes, let's indeed see what happens with it. I had also been under the impression that MakeCallback should always be safe, which is why it is used everywhere. Having to correctly choose between two different ways of calling is not really acceptable, it will lead to so many hard-to-trace runtime bugs either way one (wrongly) chooses to make the call.

[saper]

I actually ran into the problem (sass/node-sass#857) where my application uses uv threadpool to launch worker threads against a C/C++ library. The C++ library invokes certain JS subroutines. In case you are using asynchronous filesystem I/O in those you can end up in a situation where all threadpool threads are blocked (because fs I/O also uses uv threadpool worker threads at the point where non are available/runnable).

So actually I need to have next ticks invoked since otherwise my application might get stuck forever.
Yes, we are using NanCallback but that does not seem to help.

[kkoopa]

Nothing can be done for that here as far as I know. NanCallback should be the correct choice, as it invokes node::MakeCallback under the hood, which handles the tickCallback. Perhaps your problem is due to something else? How does it behave under Node 0.10? It might be a bug in libuv or Node for all I know, but I'm not qualified to determine exactly what the reason is.

[saper]

Yes, there is nothing nan can do better. We block threads in the uv threadpool; I really wish I could have coroutines there and bility to suspend worker's execution. My solution will probably a separate worker thread communicating with the library. By the way I have an idea for a better fix to TooTallNate/node-weak#35, using uv_async_t.