Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Callbacks are being GC'd #72

Closed
jahewson opened this Issue · 7 comments

2 participants

@jahewson

I've been using ffi.Callback in a scenario where the callback is called repeatedly throughout the life of the application. After a few seconds (sometimes more, sometimes less), I get an access violation with the following stack:

node!v8::Function::Call+0xd [api.cc @ 3631]
ffi_bindings!CallbackInfo::DispatchToV8+0x9a [callback_info.cc @ 46]
ffi_bindings!CallbackInfo::Invoke+0x2f [callback_info.cc @ 132]
ffi_bindings!ffi_closure_SYSV_inner+0x80 [ffi.c @ 500]
ffi_bindings!ffi_closure_STDCALL+0x22
...

The problem is that info->function is a null-pointer at the following line of callback_info.cc:

// invoke the registered callback function
info->function->Call(Context::GetCurrent()->Global(), 2, argv);

It seems that function has been GC'd, I presume this is because my callback function was GC'd as I don't have any references to it elsewhere in the code.

My workaround is to keep a reference to the callback inside a process.on('exit') callback, and this fixes the problem. Is there some way to stop V8 from GC'ing a callback which is expected to be called multiple times? It seems like this would need to be an option of Callback?

On the other hand, the workaround is perfectly acceptable: callbacks should be GC'd. Perhaps a more graceful error message in callback_info.cc is all that's needed?

@TooTallNate
Owner

Yikes, this is definitely a bug. Do you have a standalone test case that can reproduce it? That would be great, but otherwise I'll try to add something to the native tests, and try to get a fix ASAP!

@jahewson

I don't have a standalone test case, as it's a Win32 GUI which I'm interfacing with (sigh). Here's some pseudo-code for roughly what I'm doing. There's an external library which allows a callback to be register-ed, which will be called multiple times each time pump is called.

var lib = ffi.Library('lib', {
  register: ['void', ['pointer']],
  pump: ['void', []],
})

var cb = function() { ... }
var ptr = ffi.Callback('int', ['int'], cb)
lib.register(ptr)

function loop() {
  lib.pump()  // <-- this will call the registered callback (cb)
  process.nextTick(loop)
}
loop()
// `cb` falls out of scope here

What I think is happening is that because there are no JavaScript references to cb it's eligible for GC. Adding the following code resolves the problem, by keeping hold of a reference to cb inside the exit function's closure:

process.on('exit', function() {
  var x = cb
});
@jahewson

I tried to make a standalone test case, but ended up with a different (but related?) error...

var ref = require('ref')
  , ffi = require('ffi')

var funcPtr = ffi.Callback('int', [ 'int' ], Math.abs)
var func = ffi.ForeignFunction(funcPtr, 'int', [ 'int' ])

function loop() {
  for (var i = 0; i < 100; i++) {
    func(-1234)
  }
  process.nextTick(loop)
}
loop()

Running the code above eventually gives me:

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: Trying to write beyond buffer length
    at writeInt32 (buffer.js:1080:12)
    at SlowBuffer.Buffer.writeInt32LE (buffer.js:1094:3)
    at Object.set (~\node_modules\ffi\node_modules\ref\lib\ref.js:920:52)
    at ~\node_modules\ffi\lib\callback.js:57:13
    at proxy (~\node_modules\ffi\lib\_foreign_function.js:60:14)
    at loop (~\Desktop\bug.js:9:5)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

Which might be related? It guess it depends whether or not V8 knows that func has a reference to funcPtr? Also Math.absis not eligible for gc.

@TooTallNate
Owner

Ok I think I got a failing test case that matches your scenario here: https://github.com/rbranson/node-ffi/blob/5db2edf2e275b98ece1a1652e9d4329c4358b4c8/test/callback.js#L41-66

Try it out and let me know if it fails similarly.

@TooTallNate
Owner

Can you open a new issue for the Trying to write beyond buffer length thing? I looked at it really quickly and for some reason the callback is getting a NULL pointer back for the return value in some cases, which is really strange to me.

@TooTallNate
Owner

So here's the thing, we're passing of this "callback" pointer (which is a just a node Buffer instance) to some foreign C function, and really, we have no idea what it's doing with it being the scenes. i.e. we never really know when the function is "done" with the pointer, and it's specific to the function in question on a case-by-case basis.

What I'm saying is... this is a tough problem. I think the easiest thing to do is what you are already doing, just keep a reference manually until you know the C stuff is done with it (which may be never in your case). I've "fixed" the previously failing test case by documenting that it's up to the developer to retain a reference to the callback Buffer until the foreign C function is done with it: a25aad9

@jahewson

Yes, the test case you added fails similarly on Windows. I had to make this edit to get the tests to build.

As you say, it's a tough problem, because there isn't really a solution. I agree that it's up to node-ffi's users to make sure that they keep around a reference to their callback function as long as is required by the foreign function.

As we can detect when this problem arises, I've added a simple check which prints a message just before the segfault, which is a little more graceful and may save somebody a few hours with a debugger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.