Repository navigation
MakeCallback is dangerous, not only or best way of calling callback; provide direct method #284
Description
Activity
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.See also nodejs/node-v0.x-archive#9245.
@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).
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.
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".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.
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 usingNanCallbackbut that does not seem to help.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.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.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.
MakeCallback does its own error handling.
On July 27, 2015 1:07:34 PM CEST, Hank BAO notifications@github.com wrote:
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. 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(), the exception can be caught.I guess my problem may related to this issue.
Reply to this email directly or view it on GitHub:
#284 (comment)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
@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
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.@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.
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:
@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.
Reply to this email directly or view it on GitHub:
#284 (comment)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."
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:
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."
Reply to this email directly or view it on GitHub:
#284 (comment)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.
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:
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.
Reply to this email directly or view it on GitHub:
#284 (comment)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...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:
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...
Reply to this email directly or view it on GitHub:
#284 (comment)I'll close this, as it should no longer be an issue.
Nan::Callis included in 2.2 andnode::MakeCallbackis now reentrant.
NodeJS lets you invoke a "callback" (really, invoke JS code from C++ code) 2 ways:
Handle<Function> fooyou can justfoo->Call()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, callingprocess._tickCallback()and any other callbacks registered vianextTick(). 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) includeFor these cases, use of
MakeCallbackis not only incorrect but extremely dangerous. The right answer is to call the callback directly. Butnandoesn't expose a way of doing this;NanCallback::Callis actually wrappingMakeCallback. So is the right answer to avoidNanCallbackaltogether? I don't think so; it serves a very real need to deal more easily with V8's memory management; usingPersistent<Function>got harder in newer V8 (node 0.11/0.12) and so it's much nicer to useNanCallbackinstead of rawHandle<Function>. So I thinkNanCallbackshould provide separate methods for calling a callback directly and calling a callback viaMakeCallback, 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 useNanCallbackfor memory management, but then have to go to some lengths to extract the realHandle<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>withoutNanCallback. But when you do useNanCallbackand 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:
NanCallback::CallDirectthat wrapsHandle<Function>::CallNanCallback::CalltoNanCallback::CallAndEndTick(or something similarly explicit that's difficult to confuse)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?)