How to wrap this C function in an idiomatic & safe way?

help
1

I'm trying to create bindings for the rendering API in libmpv. I'm stuck on a function:

typedef void (*mpv_render_update_fn)(void *cb_ctx);
void mpv_render_context_set_update_callback(mpv_render_context *ctx,
                                            mpv_render_update_fn callback,
                                            void *callback_ctx);

Now, I have a RenderContext struct that maintains the ctx variable. But what should I do about the opaque context pointer, particularly the lifetime of it? When this function is called from C, we basically trust the caller to maintain the validity of this pointer for as long as the callback might be called.

My first thought is using Rc with a generic type, so something like this:

impl RenderContext {
    pub fn set_update_callback<Context, Callback>(&self, callback: Callback, ctx: &Rc<Context>)
    where Callback: Fn(Rc<Context>) {
        // call mpv_render_context_set_update_callback with a wrapper 
        // that translates between the Rust type and void*
    }
}

But using Rc feels like an overkill. Is there a better way?

2

Usually, in a case when C api takes a callback with a void * context, this translates into closure's environment on Rust side (unless you want to manage the context manually).

In your snippet there are actually two contexts here Fn(Rc<Context>):

  • The explicit Rc<Context>,
  • and the implicit environment of the closure, which (if you want to support closures), mpv also needs to also somehow access. This could be removed by requiring regular function (callback: fn(Rc<Context>)) instead of Fn closure.

Here's how I'd approach it: playground. I've removed the Context parameter, and also added a 'static requirement for closure, to simplify lifetime management.

If the caller needs some Rc<Context>, they could move such a thing into closure's env.

After writing this response I realized this doesn't really answer your question:

But what should I do about the opaque context pointer, particularly the lifetime of it? When this function is called from C, we basically trust the caller to maintain the validity of this pointer for as long as the callback might be called.

Not sure if there's any way to check validity of this pointer. Probably, we should trust the C code to behave? I guess this is highly library-dependent. Regarding the usage of Rc, I guess this won't really help, as C code doesn't know how to manipulate the refcount, as it sees only *mut c_void.

3

@tamas does the C API have a way to unregister a callback?

4

Thanks! Your answer led me to this blog post that explains this neatly: Neat Rust Tricks: Passing Rust Closures to C

Not sure if there's any way to check validity of this pointer. Probably, we should trust the C code to behave?

This pointer is meant to point to something on the API-user's side, i.e. the caller is responsible. So my question was about managing the lifetime of the context variable. But you answered that above, so now I know what to do. Many thanks!

1 Like
5

It doesn't, as far as I know. It does have a function to destroy the render_context which I assume will "unregister" the callback.

6

That's a pity. Know that in that case, @krdln solution is unsound, since the captured pointer ends up dangling when the rusty wrapper is dropped.

Sometimes there is at most one callback to be registered, in which case you can override a registered callback with a dummy no-op one, which acts as deregistering it.

Otherwise, unless you have access to some other guarantees, you should be conservative and take an &'static (impl Sync + Fn()) parameter, i.e., a: <F : 'static + Sync + Fn> f: &'static F.

For stateless closures, this works as is, otherwise (stateful closures) the caller would be needed to, for instance, Box::leak(Box::new(move || ...)) (you can take a f: F rather than : &'static F and do the Box::leaking yourself inside your exposed function's body, but although ergonomic, that will prevent you from sharing the callback in other places).

If you know no parallel accesses will happen / the callback does not need to be thread-safe, you can drop the Sync bound.

  • If on top of that, you know no concurrent calls will ever happen (e.g., re-entrant code), you can use FnMut() + Send instead (with an &'static mut for the reference case).

    • If, on top of the two previous things, you know these sequential calls will happen on the same thread where the registration happened, you can drop the Send bound.

Another approach, since the registering callback seems to take a render_context, is to try and tie the callback to the lifetime of that render context (at least that of the rusty wrapper): take an Arc<impl 'static + Fn() + Send + Sync> (or an Rc<impl 'static + Fn()> if thread-bound / single-threaded), downgrade it to a Weak which is the raw pointer that gets used as the ctx, and have your rusty wrapper of the context hold the owning {A,}Rc on its own: as long as the context lives, so will the closure, and when the rusty context dies, that {A,}Rc is dropped, invalidating the C callback in a controlled manner (when that C callback tries to upgrade the ctx, if it fails, it can simply no-op (or, when available, return some error code)). This reduces the "leakage" to the some heap-allocated memory big enough to hold two counters, and the shallow no-indirection size of the closure captures (if the shallow size is big, then you can {A,}Rc<Box<F>>, so that that shallow size is just that of a pointer, leading to leaked Weak leaking just 3 usizes worth of memory, so 24 bytes). So, for instance, if the closure captured a file handle or a socket handle, those would be properly closed.

TL,DR

As you can see, there are no pretty solutions, but that's because the C API is already quite flawed to begin with (which is, sadly, often the case).

2 Likes
7

Thanks for the analysis. Looking at the API more, I think the expectation for users is to either keep one context around for the complete run of the program, or create/destroy one as needed. Destroying the context means deregistering the callback because (apparently) the context is stored as a "private field" in the opaque data struct that represents a context: https://github.com/mpv-player/mpv/blob/b961efb0af166fddb2513ad0af210f956710e0ee/video/out/vo_libmpv.c#L245

8

In that case, and given that:

https://github.com/mpv-player/mpv/blob/b961efb0af166fddb2513ad0af210f956710e0ee/video/out/vo_libmpv.c#L644-L651

seems to be the only place that calls the callback, then @krdln solution, with a Send bound added for good measure, seems like the best one here :slightly_smiling_face:

2 Likes
9

Thanks, I really appreciate the help!

1 Like
10

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.