How to store a Lua function in UserData

[Timidger]

I need to store a user-provided Lua function in a struct that implements UserData. I tried using UserDataMethods to add a method that takes in a function and stores it but I keep running into lifetime errors.

rlua::Lua is expected to live for the entirety of the program (and in fact I put it in a lazy_static! so that its address is always stable), but I can't find a way to indicate in the custom user data method that rlua::Lua will be 'static and there's no way for me to convince it that the anonymous 'lua lifetime for the passed in rlua::Lua reference will live longer than the reference I store in the UserData.

Here's a minimized example of the issue I'm running into:

use rlua;

// How long the function lives, should be == 'lua
struct MyUserData<'function> { 
    function: Option<rlua::Function<'function>>
}

// HAS to be static, due to `UserData` requirements...
impl UserData for MyUserData<'static> 
{
    fn add_methods(methods: &mut rlua::UserDataMethods<Self>) {
        methods.add_method_mut("test", test_function);
    }
}

// lifetime issue occurs in this function
// is there some way for me to prove that `this` lives as long as `lua`??
fn test_function(lua: &rlua::Lua, this: &mut MyUserData, func: rlua::Function)
                        -> rlua::Result<()> {
    this.function = Some(func);
}

fn main() {}

[jonas-schievink]

You need func: rlua::Function<'static>. lua isn't even used in that function.

[Timidger]

The problem if I do that is then I get a compile error saying it could not infer an appropriate lifetime for 'lua due to conflicting requirements. Which makes sense, the rlua::Function needs to live as long as static, but the &rlua::Lua it gets that from doesn't report to live that long. I also can't explicitly say that &'static rlua::Lua because then it won't match the signature of the function for add_method_mut.

Here's the exact code that I'm using:

//! AwesomeWM drawable interface, for all things that are drawable

use std::fmt::{self, Display, Formatter};
use rustwlc::Geometry;
use cairo::ImageSurface;
use rlua::{self, Lua, UserData, AnyUserData, UserDataMethods, MetaMethod};
use ::render::Renderable;
use super::{object, class, Signal};

pub type DrawableRefreshCallback<T: UserData> = fn (&mut T);

pub struct Drawable<T: UserData> {
    signals: Vec<Signal<'static>>,
    surface: Option<ImageSurface>,
    geometry: Geometry,
    refreshed: bool,
    refresh_callback: DrawableRefreshCallback<T>,
    data: T
}

impl <T: UserData> Display for Drawable<T> {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "Display: {:p}", self)
    }
}

impl <T: UserData> UserData for Drawable<T> {
    fn add_methods(methods: &mut UserDataMethods<Self>) {
        object::add_meta_methods(methods);
        class::add_meta_methods(methods);
        methods.add_method_mut("refresh", Drawable::refresh);
        methods.add_method_mut("geometry", Drawable::geometry);
        methods.add_method_mut("foo", |lua, this: &mut Drawable<T>, func: rlua::Function<'static>| {
            this.signals[0].funcs.push(func);
            Ok(())
        });;

and the compilation error:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'lua` due to conflicting requirements
  --> src/awesome/drawable.rs:33:17
   |
33 |         methods.add_method_mut("foo", |lua, this: &mut Drawable<T>, func: rlua::Function<'static>| {
   |                 ^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the method body at 28:5...
  --> src/awesome/drawable.rs:28:5
   |
28 | /     fn add_methods(methods: &mut UserDataMethods<Self>) {
29 | |         object::add_meta_methods(methods);
30 | |         class::add_meta_methods(methods);
31 | |         methods.add_method_mut("refresh", Drawable::refresh);
...  |
46 | |         // which is annoying but whatever.
47 | |     }
   | |_____^
note: ...so that types are compatible (expected &mut rlua::UserDataMethods<'_, awesome::drawable::Drawable<T>>, found &mut rlua::UserDataMethods<'_, awesome::drawable::Drawable<T>>)
  --> src/awesome/drawable.rs:33:17
   |
33 |         methods.add_method_mut("foo", |lua, this: &mut Drawable<T>, func: rlua::Function<'static>| {
   |                 ^^^^^^^^^^^^^^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that types are compatible (expected rlua::FromLua<'_>, found rlua::FromLua<'_>)
  --> src/awesome/drawable.rs:33:17
   |
33 |         methods.add_method_mut("foo", |lua, this: &mut Drawable<T>, func: rlua::Function<'static>| {
   |                 ^^^^^^^^^^^^^^

error: aborting due to previous error(s)

error: Could not compile `way-cooler`.

[kyren]

I might be able to fix this if I add a lifetime parameter to the UserData trait itself, give me a bit to try that out.

Just so you know though, this is almost NEVER a good idea. This bindings system is not designed to have Lua handles inside UserData, because Lua itself cannot garbage collect through a rust UserData, so you can very easily end up with unbreakable cycles.

Still, I think this is possible IF the UserData trait has the lua lifetime as a parameter, which could then be set to be 'static, let me try it.

[kyren]

Okay, there's now a branch called userdata_lifetime which adds the lifetime of Lua to the UserData trait, which is what you need to make this work.

extern crate rlua;

// How long the function lives, should be == 'lua
struct MyUserData<'function> {
    function: Option<rlua::Function<'function>>,
}

// HAS to be static, due to `UserData` requirements...
impl rlua::UserData<'static> for MyUserData<'static> {
    fn add_methods(methods: &mut rlua::UserDataMethods<'static, Self>) {
        methods.add_method_mut("test", test_function);
    }
}

// lifetime issue occurs in this function
// is there some way for me to prove that `this` lives as long as `lua`??
fn test_function(
    _: &'static rlua::Lua,
    this: &mut MyUserData,
    func: rlua::Function<'static>,
) -> rlua::Result<()> {
    this.function = Some(func);
    Ok(())
}

fn main() {}

This example now compiles, and what you want to do should now be possible, but I'm not sure actually how wise it is. Especially since this is a Function that you want to store in a UserData, you have to be 100% sure that it is not, in turn, possible to reference that UserData inside the Lua function, otherwise neither will ever be garbage collected.

I haven't merged this branch with master because again, I'm not sure how useful this actually is other than to do something that's already really dangerous, but maybe it's more useful than I'm imagining? It does actually match other traits like ToLua and FromLua that way

Also, how are you actually using lazy_static, since Lua is not Sync? I guess it would have to be thread local, right?

[Timidger]

@kyren Thanks for putting the work into make this work. I understand if you don't want this in the master branch. I'll need to think about this hard to make sure the issues you brought up don't happen.

To expand on my use case: I'm trying to make Way Cooler compatibly with the libraries for the X11 window manager Awesome.

Part of that is having "signals" for objects described in the "signals" section here. The data described is stored internally as UserData and the methods are stored either on the class themselves or on an individual instance of the object. This allows the user to override capabilities for e.g a single client or for all of them (though signals can be applied to more than just clients). Signals are triggered from other events, so they need to be stored in the UserData so that Way Cooler knows when to execute them.

EDIT: Also, if look at the documentation for Awesome closely, you'll see that the whole point of the method is to reference that userdata... so with the current setup you are correct that this will always cause an unbreakable reference cycle. Not good. The C code in the original Awesome gets around this by manually managing the reference count which you obviously shouldn't expose.

I put the rlua::Lua in a lazy_static by putting in a wrapper struct and then unsafe impl Send for LuaWrapper {}. Yeah, that's bad and potentially unsafe. However, because I store it in a lazy_static! and never move it it should be safe unless I made a terrible mistake somewhere / misunderstand what it is that rlua::Lua stores.

I do this because that's how the old Lua system in Way Cooler was designed, because Way Cooler is split into various modules that contain a lazy_static of the thing they are modifying (due to limitations when interacting with the C library that does lower level compositing).

[Timidger]

Would it be possible to avoid a cycle if on the impl for the Drop trait of the UserData we clean up the rlua::Function's it holds first in order to decrement the reference count into the parent UserData so that when the drop method of UserData is called there will no longer be a cycle?

I.E if I used Option<Function> or a Vec<Function> and always make sure to set it to None or clear out the Vec will there still be a cycle? The UserData would no longer have a reference to the Function and so there'd no longer be a cycle. And couldn't that be covered by the default drop already (which calls the destructors of the fields of a struct).

Finally, if you're worried about the safety of this it's not actually defined for memory leaks to be unsafe in Rust terms. Of course, you still don't want memory leaks, and for the majority of cases you won't need this functionality / it won't come up. So if this comes up to be a solution for me, would it be possible to add this either as is or as a separate type with big warning documentation explaining why you should be careful to avoid cycles?

[jonas-schievink]

@Timidger The problem is that Drop will only run if Lua deems the userdata unreachable and calls its finalizer, which never happens if it's already involved in a cycle.

[Timidger]

@jonas-schievink ah, ok. Then in that case would a manual clearing of the reference to the Function still work? I.E I would delete a function when the user asks me to (which is part of the API I'm building) and so then Lua wouldn't see a cycle anymore.

[jonas-schievink]

That should work, yes. When the Function object is dropped, it deletes the reference. If there are no more references to the Lua function, it can be collected.

However, this opens up the possibility of memory leaks when you forget to call the function (something Lua ought to prevent in all cases IMO).

As an alternative, you can try to use a weak table to store the associations you need. rlua doesn't let you create those (or at least not easily), but you can write some wrapper code in Lua. Now that I think about it, it should be possible to write a Lua wrapper for your stuff:delete() approach, too... (that handles calling :delete in all cases).

[kyren]

Maybe there's some way to formalize this for UserData, so that instead of trying to store handles to values inside the UserData, we simply allow the methods of UserData to get / set values in some metatable storage. That way, you can store arbitrary Lua next to userdata, and it avoids all the messy lifetime issues.

Let me play around with an API for this.

Edit: I kind of want to solve this, because this problem tends to come up a lot. Chucklefish has absolutely run into this exact userdata reference problem before, it is not specific to rlua or rust at all (which I'm sure you guys know, but for anybody following along).

[kyren]

This isn't the most ergonomic approach, but a simple solution would be to add methods to access metatable values on AnyUserData. That way, what you would do is instead of calling add_method or add_method_mut, you'd call add_function, and then accept an AnyUserData as the first parameter, and you can type check it and then get the values you need out of the metatable.

In fact, what I could do is just go ahead and add methods to change metatables on both Table and UserData, where for Table we would allow general access, but for UserData I guess we would only allow keys that don't start with double underscore? I know this isn't a perfectly ergonomic solution, but it's not too bad and it's pretty generally useful.

What do you guys think?

Edit: Also, this is basically isomorphic to not using userdata directly, and just having a Lua table with userdata and then some other data, and using functions instead of userdata methods, so something like this should already be possible to do if you don't restrict yourself to userdata methods. The features I'm suggesting are not bad though, so if this is in any way simpler, I'm happy to do it.

[kyren]

I put the rlua::Lua in a lazy_static by putting in a wrapper struct and then unsafe impl Send for LuaWrapper {}. Yeah, that's bad and potentially unsafe. However, because I store it in a lazy_static! and never move it it should be safe unless I made a terrible mistake somewhere / misunderstand what it is that rlua::Lua stores.

I also want to point out, just in case, that unsafe impl Send for LuaWrapper {} is only safe when you're sure that you don't store any handle at all across threads, and I would really hesitate to even use the word "safe", since it sounds really easy to misuse. I might be misunderstanding exactly what you're doing, but especially when you have a 'static Lua, that means handles will also have 'static lifetime, which means if you get a handle to.. anything really, and keep it on thread A, then move Lua to thread B, you're gonna have a bad time. The best way to imagine it is a messy nest of Rc and RefCell.

Edit: I apologize if you already understand all this, it's possible you've already thought of all that and / or I misunderstand.

[Timidger]

@kyren Ok, so then I could store the functions directly in the metatable? Or would I still need to use a weak table as @jonas-schievink pointed out? Sounds like a good solution though.

I also want to point out, just in case, that unsafe impl Send for LuaWrapper {} is only safe when you're sure that you don't store any handle at all across threads, and I would really hesitate to even use the word "safe", since it sounds really easy to misuse. I might be misunderstanding exactly what you're doing, but especially when you have a 'static Lua, that means handles will also have 'static lifetime, which means if you get a handle to.. anything really, and keep it on thread A, then move Lua to thread B, you're gonna have a bad time. The best way to imagine it is a messy nest of Rc and RefCell.

I store the LuaWrapper behind a Mutex, which doesn't move the content at all. Once I put it in a lazy_static! it doesn't move, ever. You're correct though that if Lua was to suddenly go away for some reason (e.g, the thread panics) then that would be unsafe because there's a dangling pointer. I'll look into making sure that doesn't occur, but for now I'm just trying to get this working ;)

[kyren]

@kyren Ok, so then I could store the functions directly in the metatable? Or would I still need to use a weak table as @jonas-schievink pointed out? Sounds like a good solution though.

No, you wouldn't need a weak table because it would just be Lua functions inside of a metatable on a userdata, all stuff that Lua's gc can see normally. Weak table support in the API is unrelated, but it WOULD be possible if I had a real metatable API on Table handles, which I was going to add as part of this.

[Timidger]

@kyren awesome! Thanks so much for helping me through this problem. You should set up a donation link, so I can buy you a beer for this 🍺

[kyren]

Okay, I looked into this a bit more, and I forgot that currently, all UserData of a single type share the same single metatable, so I can't easily do associated UserData values like I thought, without a pretty significant performance impact.

BUT, like I mentioned before, that approach is actually isomorphic to something else that's already possible. Instead of having a UserData that contains Rust values along with Lua values, and causing all these problems, simply make a Table that contains both UserData and Lua values, and add methods to that table. The part of this that's currently very annoying is that you have no API to access metatables via rust, and I'm still going to fix that, and I'll go through the situation with you to make sure that what you want to do is definitely possible.

[kyren]

Well, I've added userdata methods to rlua::Table, which should make what you want to do SLIGHTLY easier, and I've also fixed a really bad bug that I found along the way, so I'm probably going to make a 0.9.2 update for it.

I want to help you through making this work with way-cooler though, so if you aren't sure what I mean or how to go about using Table instead of UserData, please let me know.

Full disclosure though, my power may go out (again) due to hurricane Irma.

[kyren]

@kyren awesome! Thanks so much for helping me through this problem. You should set up a donation link, so I can buy you a beer for this 🍺

That's very kind of you haha, just buy me a beer if we meet IRL :D

[Timidger]

Great to hear! I'll test the changes out immediantly to make sure I have the correct idea / this fixes my problem.

Full disclosure though, my power may go out due to hurricane Irma.

Stay safe out there!

[Timidger]

@kyren

Ok, using the latest master I whipped this up.

#[derive(Clone, Debug)]
pub struct Button {
    num: u32
}

impl Display for Button {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "Button: {:p}", self)
    }
}

impl UserData for Button {
    fn add_methods(methods: &mut UserDataMethods<Self>) {
        object::add_meta_methods(methods);
        class::add_meta_methods(methods);
    }
}

/// Makes a new button stored in a table beside its signals
pub fn new(lua: &Lua, num: u32) -> rlua::Result<Table> {
    let button_table = lua.create_table();
    button_table.set("data", Button { num })?;
    let meta = lua.create_table();
    meta.set("__index", lua.create_function(default_index))?;
    meta.set("signals", lua.create_table());
    meta.set("__tostring", lua.create_function(|_, button_table: Table|
                                               Ok(format!("{}", button_table.get::<_, Button>("data").unwrap()))));
    button_table.set_metatable(Some(meta));
    Ok(button_table)
}

fn default_index<'lua>(lua: &'lua Lua, (button_table, index): (Table<'lua>, String))
                 -> rlua::Result<rlua::Value<'lua>> {
    if let Ok(val) = button_table.get::<_, rlua::Value>(index.clone()) {
        return Ok(val)
    }
    // TODO error handling
    let button = button_table.raw_get::<_, Button>("data").unwrap();
    match index.as_str() {
        "connect_signal" => {
            lua.globals().set("__temp", button_table);
            Ok(rlua::Value::Function(lua.create_function(|lua, val: rlua::Value| {
                let button_table = lua.globals().get::<_, Table>("__temp").unwrap();
                lua.globals().set("__temp", rlua::Value::Nil);
                let signals = button_table.get_metatable()
                    .expect("no meta")
                    .get::<_, Table>("signals")
                    .expect("signals was not a table");
                signals.set(signals.len().expect("No length"), val);
                Ok(())
            })))
        },
        "emit_signal" => {
            lua.globals().set("__temp", button_table);
            Ok(rlua::Value::Function(lua.create_function(|lua, val: rlua::Value| {
                let button_table = lua.globals().get::<_, Table>("__temp").unwrap();
                lua.globals().set("__temp", rlua::Value::Nil);
                let signals = button_table.get_metatable().unwrap().get::<_, Table>("signals").unwrap();
                signals.get::<_,rlua::Function>(0).unwrap().call::<_,()>(button_table);
                Ok(())
            })))
        },
        "num" => Ok(rlua::Value::Number(button.num as _)),
        // TODO Error here
        _ => Ok(rlua::Value::Nil)
    }
    // TODO special "valid" property
}

pub fn init(lua: &Lua) -> rlua::Result<()> {
    unimplemented!()
}


mod test {
    use rlua::*;
    use super::*;
    #[test]
    fn basic_test() {
        let lua = Lua::new();
        lua.globals().set("button0", new(&lua, 0).unwrap());
        lua.globals().set("button1", new(&lua, 1).unwrap());
        lua.eval(r#"
                 print(button0)
                 print(button0.num)
                 print(button1.num)
                 print(button0.connect_signal(function(button) button.num = 3 end))
                 print(button0.emit_signal())
                 print(button0.num)
"#,
        None).unwrap()
    }
}

output:

running 1 test
Button: 0x7fd6fb9fb8f0
0.0
1.0


3

This is super ugly, and I can probably find better ways to do things (like that horrible __temp value I store in Lua so that I can immediately call the signal function with the table in the context...I'm ok if that's the only solution, but it bothers me slightly I have to do that).

But hey, it works and that makes me happy 😁.

Thanks again so much for working through this with me, this is the last nice feature rlua needed for me to justify using this over the raw C Lua bindings (which are way too low level, this is much nicer).

[kyren]

I'm glad you were able to work through it, I'm sorry that the table method is a bit verbose. There's this whole API for adding methods to userdata and absolutely none for tables, so it's a tad annoying that it takes so much work to do the same for a table. I'll try and come up with some way to make this easier, and maybe have the metatable API work for both. I'm convinced though that storing lua values inside lua is the right approach as opposed to storing lua handles in a userdata, so this is at least in the right direction.

[Timidger]

I finished up my implementation that relies on this (as you can see in the linked PR). Any update on when this will be released in a v0.9.2?

[kyren]

Ugh, sorry for taking a while on that, I'll do it today. I still need to merge a few PRs still and also write a changelog including changes back in history before I do another release.

[Timidger]

No worries, take your time. I probably won't merge my branch immediately since I still want to review it and maybe add on to it. Just wanted to check the status / make sure the API wasn't going to change.

[kyren]

I might actually make a change to the API in the near future, in regards to issue #38, but it won't be in the 0.9 version. We follow pre-1.0 semver (mostly).

[Timidger]

Awesome looks like this has been released with 0.9.2. I'll go ahead and close this. Thanks again!

[Timidger]

Sorry to bump this old thread but I have a question now after reading through this again and after discussing my design with @psychon over on the Way Cooler gitter channel.

Is there a reason this problem (of storing Lua data alongside Rust user data) can't be solved by exposing a way to set the user value ala lua_setuservalue? Is there a safety hole by binding a table to a user data that I'm not aware of?

[jonas-schievink]

Seems like that's the canonical solution to this problem. The uservalue is a transparent reference that can be seen by the GC, which solves the problem outlined above.

[kyren]

The reason I didn't implement an API using lua_setuservalue is simply because I didn't know it existed... TIL.

I'll implement something using this shortly. What would you guys prefer, an API to set a single arbitrary Lua value as the associated user value, or an API that assumed a table? Considering you can implement the second in terms of the first, I would assume the first would be better, but it's a bit more work for the obvious use cases.

[kyren]

Okay, the simpler version of that API is added.