Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safer context for closures #125

Open
kornelski opened this issue Aug 15, 2019 · 8 comments
Open

Safer context for closures #125

kornelski opened this issue Aug 15, 2019 · 8 comments
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@kornelski
Copy link
Collaborator

kornelski commented Aug 15, 2019

Currently gifski C API takes a callback function + a context pointer. From Swift's perspective this has to be a plain, context-free function, and the context pointer is meaningless (unsafe unretained). That makes it awkward to use (doesn't allow normal closures), and is unsafe.

Can we do better?

I don't know what's Swift solution to the memory management problem here. In the olden days of manual refcounting, it'd suffice to call an extra [context retain] before setting up the callback's context, and [context release] after finishing the operation. Rust's approach to this is into_raw() and from_raw() that "leak" memory and "unleak" it later.

The second part of the problem is allowing normal closures, instead of sneaking the context through an unsafe pointer.

  • 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.

  • 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.

@sindresorhus sindresorhus added help wanted Extra attention is needed question Further information is requested labels Aug 15, 2019
@ghost
Copy link

ghost commented Aug 15, 2019

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

@kornelski
Copy link
Collaborator Author

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

@ghost
Copy link

ghost commented Aug 16, 2019

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 ?

@kornelski
Copy link
Collaborator Author

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.

@sindresorhus
Copy link
Owner

@sindresorhus
Copy link
Owner

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.

@kornelski
Copy link
Collaborator Author

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.

@kornelski
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants