Hacker News new | ask | show | jobs
by Diggsey 2612 days ago
I can't speak to every issue which the author might have encountered, but there is a better solution to the lifetime management problem than the two mentioned in the article.

Instead of this:

    fn some_wlroots_callback(output_handle: OutputHandle,
                             surface_handle: SurfaceHandle) {
        output_handle.run(|output| {
            surface_handle.run(|surface| {
                // maybe some more nested layers...
            }).unwrap()
        }).unwrap()
    }
One can do this:

    fn some_wlroots_callback(
        ctx: CallbackContext,
        output_handle: OutputHandle,
        surface_handle: SurfaceHandle
    ) {
        let output = ctx.get(output_handle);
        let surface = ctx.get(surface_handle);
    }
This is safe, because the lifetime of "output" and "surface" can be bound to the lifetime of the "ctx" (whose lifetime is controlled by the library: the library simply has to make sure that "ctx" is not accessible outside of a callback).

edit: Realised you can't tell that there's an implicit lifetime in `CallbackContext` here:

    struct CallbackContext<'a> { ... };
OR

    type CallbackContext<'a> = &'a CallbackContextImpl;
2 comments

Author here.

The problem with that design (which is a great design given what I presented in the article by the way!) is that it doesn't allow you to share handles across callbacks, which is mandatory to do anything interesting. I'm assuming here that you can't use the handles except for that callback context. If you can, then that presents a different problem.

https://play.rust-lang.org/?version=stable&mode=debug&editio...

If you can own a context, even with a lifetime parameter it's possible to leak it using the Box api. That allows you to have access to a &'static CallbackContext. This will break that assumption that it only lives as long as the callback itself.

The intent is that handles can be used across contexts (as long as both contexts are from the same instance of wlroots) - the context checks that the handle is valid before unwrapping it. Could you clarify what you mean by it presenting a different problem?

With regards to the leaking issue - that's not actually correct. `Box::leak` does not give you access to the context with the `'static` lifetime, assuming the lifetime of the context is not 'static (that would fundamentally break rust's safety guarantees).

The only potential problem is that the destructor for the context may not run (cf. leakpocalypse). If the implementation requires the destructor to run, then you are right, you should never give ownership of the context object, it should be passed into the callback by reference. If you need access to a context outside of a callback, then you should use the callback-style:

    with_context(|ctx| {
        ... do stuff ...
    }
The nice thing is that `with_context` can enforce any threading constraints you might have (like contexts being accessed from a single thread), can ensure that the context is safely dropped at the end, and it's only required once - once you have a context you can use it as many times as you need.
Your edit to your original comment was important information I was missing - you are correct that that would probably work.

With how wlroots-rs is setup it does require the upgraded resources to not be leaked because their drop impl decrements the reference count.

Regardless, this was obviously only one part of the problem I was running into. As well, as can be seen, it's very complicated trying to ensure this is all sound and it's no longer worth the headache.

>If you can own a context, even with a lifetime parameter it's possible to leak it using the Box api.

The `std::pin::Pin` API prevents you from doing this (callbacks would receive `Pin<&'_ mut CallbackContext>`).

>The problem with that design (which is a great design given what I presented in the article by the way!) is that it doesn't allow you to share handles across callbacks

But how does sharing handles between callbacks look like? Is it something like "callback FooCreated provided me a Foo handle" and "callback BarCreated requires me to have the Foo from the FooCreated callback" ? So your FooCreated callback needs to save the Foo handle somewhere that it can be reused later?

If so, why is it not enough to have a `fn CallbackContext::set_foo_handle(&mut self, Foo)` (or `Pin<&mut self>` based on the above suggestion) ?

If you want to see some examples, Neon uses this pattern to solve a similar problem - preventing rust code from holding onto js objects that might get gced between callbacks.

https://github.com/neon-bindings/neon

I just want to say, having spent a bunch of time writing a context-management framework for embedded targets that I jettisoned because it sucked, this is a great approach I'm excited to experiment with. Thank you for the comment.