node::MakeCallback is dangerous, can you detect misuse and warn?

[metamatt]

This issue springs from TooTallNate/node-weak#35, but the root cause is that node::MakeCallback is dangerous if misused, and it would be nice to prevent its misuse.

By "misuse", I mean: if you have a Handle<Function> in a node addon and you want to call it, you have the choice of calling it directly (Handle<Function>::Call()) or via node::MakeCallback. You should do the former if your addon code was invoked from JS code; you should do the latter if your addon code was invoked directly from libuv as an event callback. This difference, while crucial, is not plainly obvious, and if you get it wrong, the results are really bad (preemption of Javascript code, that breaks Node's entire threadless/concurrency/consistency model).

Note comment TooTallNate/node-weak#36 (comment) from @trevnorris (and the surrounding discussion) saying that MakeCallback should be benign if called unnecessarily, but in my observation, that's not true. So I'm following up on this old discussion with an issue report and demo/repro case (for NodeJS 0.12.0) that shows it's actually (and still) dangerous if called this way.

The repro case is at https://gist.github.com/metamatt/4ce96adbf14de9a97d41.

[trevnorris]

@metamatt Thanks. There's a simple fix, but going to investigate to make sure it's also the best.

[trevnorris]

So, a conceptually simple fix. Working out the details.

[kkoopa]

I hope the solution will make MakeCallback always work, as intended. Having to correctly decide between two different ways of calling javascript from C++ dependent on context is really not acceptable, as it will lead to much confusion and headache.

[trevnorris]

@kkoopa That's the plan. :)

[misterdjules]

Adding to the 0.12.1 milestone, as discussed during the last core team meeting.

[johnhaley81]

Will this be back-ported to 0.10?

[kkoopa]

NAN will most likely reimplement and backport all of it, as I don't imagine 0.8 getting updated, so 0.10 will at least be covered that way.

[misterdjules]

@trevnorris Any update on this?

[trevnorris]

@misterdjules No, sorry. I've taken several stabs at it but every time I've been thwarted by some edge scenario. I'll take another look this week and see if anything new pops to mind.

[kkoopa]

What is the status on this? Is it doable or should it be resolved some other way?

[trevnorris]

@kkoopa Ooh. Just had an idea. Something like:

bool in_make_callback = false;
if (!env->in_make_callback()) {
  env->set_in_make_callback(true);
  in_make_callback = true;
}
// perform operations 'n stuff
if (in_make_callback) {
  env->set_in_make_callback(false);
}

So the call can keep track of whether it's in a MakeCallback sequence or not. Don't have the mental energy to do this today, but I'll give it a shot tomorrow.

[kkoopa]

Cool. Hope it works.

[trevnorris]

I still haven't found a way to fix all the edge cases. The rules for calling it are pretty simple. It has to be the first entry into the JS stack. I'm going to keep thinking about it though. Feel like there's got to be a cleaner architecture here.

[metamatt]

Thanks for sticking with this. To reiterate why it's important, I'll link to the nan documentation:

Because node::MakeCallback() now takes an Isolate, and because it doesn't exist in older versions of Node, we've introduced NanMakeCallback(). You should always use this when calling a JavaScript function from C++.

Having two different mechanisms ("synchronously invoke JS code" from C++ code invoked synchronously, which is Function::Call or NanCallback, and "invoke JS code plus signal end of tick" from C++ code invoked asynchronously, which is MakeCallback or NanMakeCallback) just doesn't seem that bad to me if documented appropriately.

[trevnorris]

Ah yeah. That documentation is worded poorly.

[kkoopa]

If one of you were to submit a PR with better documentation, I'd be happy to merge it, assuming this is the official approach (for the time being).

[trevnorris]

@metamatt I have a partial fix for this issue. My final issue to resolve is the fact that the initial set of callback don't run in a MakeCallback. So it's no use in that case to determine whether they're already being run. Haven't found a solution to this, but still looking.

[trevnorris]

This may finally be fixed by nodejs/node#4507