node-weak causes JS code preemption due to MakeCallback #35
Comments
|
Hey @TooTallNate, what's your protocol for accepting contributions to this repo, how do I build and test? I have a fix for this I'd like to propose. My first assumption was "check out the source repo, run So then I just did I see the appveyor.yml which I assume might automate some of this but I don't know anything about appveyor. |
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.
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 TooTallNate#35.
|
Nice detective work here :) |
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 TooTallNate#35.
|
I think I'm seeing something similar on io.js 1.2.0 using |
|
Yeah, the issue is real and will affect all nodejs versions and iojs versions, and it's not dependent on the version of nan. (While I did request a change to nan, which if merged can make my patch to node-weak look nicer, the change to nan alone will not be enough; this needs a patch to node-weak.) |
|
Correction: the issue is no longer real. Excellent. Thanks @TooTallNate. |
(Pre-disclaimer, apologies if anything I say here is incorrect or overly alarmist. I think this is a real problem but I don't pretend to understand everything that's going on here, especially the Node and V8 codebase I'm not very familiar with.)
I've seen some problems in my NodeJS app using node-weak which seemed like they could only be explained by my code getting preempted by other JS code that changed state out from under it. I believe I've eventually traced this to the use of MakeCallback in Node's native code (src/node.cc); when it's called, it calls not only the callback you told it to call but also all registered nextTick callbacks. And node-weak invokes MakeCallback from GC, which basically means, from anywhere.
Here's a demo of the problem.
I started a thread about this on the NodeJS mailing list, too.
To the best of my evolving understanding, either
I don't mean to malign node-weak; it's very cool and very useful functionality, but this side effect of preempting code and executing pretty much arbitrary any other code during GC is quite alarming.
The text was updated successfully, but these errors were encountered: