Safer context for closures #125

Looking at the code I can see the closures are marked with @convention(c).

...this convention disallows the closure from using any captured variables and limits what the callback can interact with in the swift domain.

I've noticed you can't simply pass the context into a custom queue either.

The solution is to use the old-style callback context mechanism (on the Swift side), pass the context with the callback to C and get C to call the callback passing the context as an argument.

I'm not skilled in the following language so I can't debug it. However, there is some inline documentation "get refcount++ without dropping the handle".

#[no_mangle]
pub extern "C" fn gifski_set_write_callback(handle: *const GifskiHandle, cb: Option<unsafe fn(usize, *const u8, *mut c_void) -> c_int>, user_data: *mut c_void) -> GifskiError {
    if handle.is_null() {
        return GifskiError::NULL_ARG;
    }
    let cb = match cb {
        Some(cb) => cb,
        None => return GifskiError::NULL_ARG,
    };
    // get refcount++ without dropping the handle
    let g = unsafe {
        let tmp = Arc::from_raw(handle);
        let g = Arc::clone(&tmp);
        let _ = Arc::into_raw(tmp);
        g
    };
    let writer = CallbackWriter {cb, user_data};
    *g.write_thread.lock().unwrap() = Some({
        let g = Arc::clone(&g);
        thread::spawn(move || {
            gifski_write_sync_internal(&g, writer, None)
        })
    });
    GifskiError::OK
}

Does this effectively mean "Increase the retain count so we don't release the closure early" ?

Reference 1 Reference 2

That refcount is for GifskiHandle, but not for the callback context.

I suppose this thread is focusing on the Swift to C bridge specifically and given that the thread is here and not here, your intention is to improve the Swift side and not the C side? That would make sense because it seems the limitations of Swift are pretty clear, when it comes to importing C functions.

It was loosely discussed in my latest pull request that this file or the whole wrapper, should be refactored to manage its own state.

I would suggest including the management of retain / release during that refactoring job.

Given that we are in an ARC world, let's just keep it simple and use strong pointers, similar to the fix here. Closure's in Swift are also first class citizens so you can be as creative as you like I guess.

I suppose the question then is, how should the public interface look for the new wrapper ?

Right, the common language for Rust and Swift is C, and C can't to anything smarter than function pointer + context pointer, so that's the interface we have to work with. Therefore handling of Swift closures well requires changes on the Swift side.

Relevant, but still far off:

• https://github.com/rust-lang/rust/pull/64582
• https://github.com/rustswift/swift-bindgen

Perhaps closures could be cast to an ObjC block? Blocks can be represented as regular pointers. The callback function could be just a small function that calls a block, and the block would be passed through the context pointer.

Do you have an examples or links about that?

Or can Swift "box" a closure into a container that can be sent as a single pointer? It could work similar to blocks, but with "native" Swift closure.

How would that look like?

It was loosely discussed in my latest pull request that this file or the whole wrapper, should be refactored to manage its own state.

We should definitely do this.

ObjC blocks ^{} are NSObject. The spec for this is probably too low-level to be useful. But just based on assumption they're NSObject, they should support passing by regular pointer as well as manage reference count the same way.

I don't know enough Swift to know how Rust's equivalent of Box would look like, but perhaps it could be just a class that contains the callback as a field?

class CallbackWrapper {
   var callback /*???*/
}

and then CallbackWrapper would be a regular object, and could be passed around and memory-managed like one.