Repository navigation
MakeCallback is dangerous, not only or best way of calling callback; provide direct method #284
Description
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> fooyou can justfoo->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::CallDirectthat wrapsHandle<Function>::Call - rename
NanCallback::CalltoNanCallback::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?)
Activity
kkoopa commentedon Feb 18, 2015
Hmm, this is actually the first time I've heard of this problem. My experience is completely opposite in that not using
node::MakeCallbackas of 0.11 would lead to a whole bunch of strange errors exactly becauseprocess._tickCallback()was not called and the same goes for domain support. Normally, every call would want to be done throughnode::MakeCallbackfor this reason.Might introduce
NanCallback:: CallDirectfor convenience, but renaming or deprecatingNanCallback::Callseems less useful. The documentation could however be improved.metamatt commentedon Feb 18, 2015
See also nodejs/node-v0.x-archive#9245.
metamatt commentedon Feb 18, 2015
@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
MakeCallbackover-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 callMakeCallbackin 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 commentedon Feb 18, 2015
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 commentedon Feb 18, 2015
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
MakeCallbackshould 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 commentedon Feb 18, 2015
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 commentedon Apr 25, 2015
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
NanCallbackbut that does not seem to help.kkoopa commentedon Apr 25, 2015
Nothing can be done for that here as far as I know.
NanCallbackshould be the correct choice, as it invokesnode::MakeCallbackunder the hood, which handles thetickCallback. 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 commentedon Apr 25, 2015
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.Handling exceptions.
hankbao commentedon Jul 27, 2015
I've just encountered a related issue. We're currently using electron to pack our web app. There's a native node addon providing some functionality for the electron app. Our web app will call the addon and pass a js callback to it. The native addon store the callback in a NanCallback. It setups a TryCatch before calling the callback. However the TryCatch always fails to catch the exception thrown by the js callback. It turns out that if I call the callback with Function::Call instead of with NanCallback::Call(), exceptions can be caught.
I guess my problem may related to this issue.
kkoopa commentedon Jul 27, 2015
MakeCallback does its own error handling.
On July 27, 2015 1:07:34 PM CEST, Hank BAO notifications@github.com wrote:
hankbao commentedon Jul 27, 2015
Unfortunately, when an exception was thrown in the js callback and caught by MakeCallback in my addon, codes doing cleanup stuffs after calling the callback won't be executed.
35 remaining items
trevnorris commentedon Oct 26, 2015
@kkoopa The patch is pretty small, but does require a patch to v8. Not sure if it's 100% correct, but has worked well enough for my testing. Not sure if it can work on v0.8. Here's the patch: https://gist.github.com/trevnorris/4802d21ba3044a0c30b0
kkoopa commentedon Oct 26, 2015
Aww, patching V8 is not really possible, save for internal leak testing. Maybe it is possible to hack something up by wrapping a
v8::HandleScopein aNan::SealHandleScopeand doing the checking by hand. I'll take a look, but don't feel optimistic. I really wish C++ had a proper way of overriding access modifiers.trevnorris commentedon Oct 26, 2015
@kkoopa sorry, the patch is actually only for v0.12. don't have a working one for v0.10. I can submit it to land, but w/o v0.10 and v0.8 support I don't think it'll be worth it.
kkoopa commentedon Oct 26, 2015
Nah, not here and now, but do submit it for 0.12 anyway, hopefully we will get to drop 0.8 and 0.10 in 12-18 months (and start forcing Isolate passing everywhere and other good stuff).
On October 26, 2015 10:29:11 PM GMT+02:00, Trevor Norris notifications@github.com wrote:
bnoordhuis commentedon Oct 27, 2015
Apropos v0.8, what's the reason nan still supports it? Upstream no longer does. I'd just sunset the support and say "nan version $x is the last one to support v0.8."
kkoopa commentedon Oct 27, 2015
It's so similar to 0.10 that it is trivial to support. Npm also support 0.8 for a while more. My plan was to drop both 0.8 and 0.10 at the same time, since nothing is gained by dropping only 0.8.
On October 27, 2015 1:47:21 PM GMT+02:00, Ben Noordhuis notifications@github.com wrote:
bnoordhuis commentedon Oct 27, 2015
It would give people an incentive to upgrade to a version that's still supported by upstream. Although if you're still stuck on v0.8 at this point, you're probably not a very upgrade-y person anyway.
kkoopa commentedon Oct 27, 2015
Yeah, but it is not so much for end-users as for developers. Their modules can run on 0.8, even if the majority of their users couldn't care less. That makes upgrading their NAN version a non-issue, giving support for new versions at no expense.
From NAN's perspective there are three versions at the moment: 0.8 to 0.10, 0.12 to io.js 2, and io.js 3 to Node 5+. These correspond to: ancient, isolates, maybes.
On October 27, 2015 1:55:03 PM GMT+02:00, Ben Noordhuis notifications@github.com wrote:
added KickNextTick
LinusU commentedon Nov 16, 2015
I got bit by this today as well, I would love to see
NanCallback::CallDirectimplemented. Would a pull request be accepted?I'm also in favor for renaming
Callto something more explicit, but that's step 2. I think we should add CallDirect either way...kkoopa commentedon Nov 16, 2015
I don't think adding that is the right approach. Just get the underlying function and use Nan::Call on it once that has been added from another PR.
Nan::Call(*callback, Nan::GetCurrentContext()->Global(), 0, 0);
On November 16, 2015 6:59:00 PM GMT+02:00, "Linus Unnebäck" notifications@github.com wrote:
LinusU commentedon Nov 17, 2015
@kkoopa Perfect, that's exactly what I need!
kkoopa commentedon Nov 17, 2015
For reference,
Nan:Callis #496, which is merely a wrapper forv8::Function::Call.Nan::Callbackalready supports the*operator as a shorthand forNan::Callback::GetFunction.kkoopa commentedon Feb 21, 2016
I'll close this, as it should no longer be an issue.
Nan::Callis included in 2.2 andnode::MakeCallbackis now reentrant.