Call callback directly to avoid unwanted preemption.

[metamatt]

Nan's NanCallback::Call goes through Node's MakeCallback, which is
designed to be used only by event callbacks (i.e. functions called
from the libuv event loop), not invoked synchronously from a call
stack where JS code was already running. This is because when
MakeCallback returns it assumes it's the end of the current "tick", so
it calls not only the callback you told it to, but also all tick
callbacks registered for the current tick.

Since node-weak's TargetCallback is invoked from GC, which could be
invoked from anywhere at any time, the code it calls into (the
callback attached to each weakref object) needs to be designed and
written carefully, essentially to the standards of signal or interrupt
handlers, since they preempt any JS code that was already running when
GC kicked in.

This is fine as long as the weakref callbacks are written to this
standard, but it also means TargetCallback needs to avoid accidentally
calling other callbacks that weren't written to this standard.

The fix is to call the callback directly, instead of via MakeCallback.

Fixes #35.

[TooTallNate]

@trevnorris @rvagg Do you guys have any comments on the correctness of this? The test failure on node v0.11.x is just a false-negative from AppVeyor. The Travis tests are all passing.

[trevnorris]

node::MakeCallback() generally should only be used when re-entering JS from an asynchronous call. I say "generally" because there might be a case when you should (e.g. process.domain is changed from C++, but hopefully that's never done).

[metamatt]

@trevnorris That's basically what I learned/concluded with my brush with MakeCallback. (I started out with a post to the nodejs mailing list before I realized the only way I was triggering that problem was node-weak, and filing #35).

Aside from node-weak, but following on what you just said about the proper use of node::MakeCallback, it might be cool if node::MakeCallback could detect and complain (abort) if there was already JS code on the stack that it's preempting, but that might not be an easy notion to express in code (especially given the domains exception you just mentioned). Or jus the name -- I don't know whence the name MakeCallback derives, but it might be cool to call it something like InvokeCallbackAndEndTick.

[trevnorris]

@metamatt There are guards in place that should prevent issues if it's called unnecessarily. If it's not then we don't have the proper guards in place. The only impact calling MakeCallback() unnecessarily should be a small performance hit.

[metamatt]

@trevnorris Please stop me if I'm wrong: I see effects much worse than that. Check out my preemption demo (single file, then you need to npm install weak and run as node --expose-gc preemption.js). That demo leverages the fact that the current node-weak callbase is indeed (via nan) calling MakeCallback unnecessarily, and in my experience that causes preemption that basically breaks the nodejs concurrency model. I've tried this with nodejs 0.10.26, 10.10.32, and a "0.11.14-pre" build I compiled myself from the "0.12" branch in late August, and get the same results. I haven't tried 0.11.14 though.

[trevnorris]

@metamatt I'll take a look into this. Not this week, but this is something that needs to be addressed.

[TooTallNate]

Not this weak

Sorry, couldn't resist :D

[metamatt]

Cool, no rush. If it's survived this long in its current state, I think it's more important to get it right than to get it changed immediately. Glad to have your attention on it (both @trevnorris, @TooTallNate).

Most of the discussion here has been about the design/usage/safety of node::MakeCallback itself, not how node-weak is using it and how I'm proposing to fix that.

• So back to the present PR, does this look correct and good?
• Would it help if I filed a separate issue against node itself for the MakeCallback API safety question?

[rvagg]

You're just going to get into trouble with 0.11,0.12 support here aren't you? we had to pull MakeCallback into NAN because of the introduction of Isolates and you still have that problem here I think. If you have trouble finding a single way to support 0.10 and 0.11 with direct callbacks then file an issue with NAN and we'll look at getting something in.

[metamatt]

@rvagg: I'm certainly no expert in the difference between node and V8 API versions that NAN handles. But the patch I'm proposing here seems to work fine with node 0.11.14 (as well as 0.10.32 and 0.10.26).

Looking at the NanCallback code, my patch is leveraging it heavily, trying to get the same underlying callback that NanCallback::Call would pass to node::MakeCallback then call it directly instead of actually using MakeCallback -- I get the context via NanGetCurrentContext which seems to abstract around the introduction of Isolates, then get the actual callback with NanCallback::GetFunction, which... well, I don't understand the difference between the 3 different ways NanCallback::GetFunction does it (NanEscapeScope(NanNew(handle)->Get())), the way NanCallback::Call does it for 0.11 (NanNew(handle)->Get()), and the way NanCallback::Call does it for 0.10 (just handle->Get()). Also when I originally authored this patch in the project I discovered the bug in, the version of nan.h that happened to be in the node_modules tree was 0.8 instead of the current 1.2.0; NanGetCurrentContext didn't exist and NanCallback::GetFunction looked pretty different (NanEscapeScope didn't exist).

So that's all to say that the patch I'm proposing here seems to compile and work fine against current 0.10 and 0.11 releases, but I don't fully understand and might be missing the concern you're raising. And is it ok to use GetFunction the way I am; why can't NanCallback::Call just use GetFunction (or why the 3 slightly different ways of wrapping handle->Get?)?

Maybe NanCallback should offer separate CallDirect and CallViaMakeCallback variants (better name suggestions welcomed) to make it obvious that the difference exists and not all clients want the MakeCallback variant.

[metamatt]

Yes, I think it's time to get this PR merged please. Nobody should be using node-weak without this fix.

I'm using this same patch with Node.js 0.12.0 and it's working fine.

I filed nodejs/nan#284 asking that nan make the correct version of this code easier to write (and less likely for people to accidentally use MakeCallback).

[metamatt]

To close the loop on the discussion in #36 (comment), I filed nodejs/node-v0.x-archive#9245.