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

Storing Table references alongside Lua. #57

Closed
dkushner opened this issue Dec 5, 2017 · 13 comments
Closed

Storing Table references alongside Lua. #57

dkushner opened this issue Dec 5, 2017 · 13 comments

Comments

@dkushner
Copy link

dkushner commented Dec 5, 2017

I'm having a bit of difficulty architecting a scripting extension to my entity component system using rlua to support user-scripted behaviours. The idea is to have the user define a table in a Lua script resembling the following:

local Test = { }

function Test:Init() 
  -- Do some stuff
end

function Test:Update(delta) 
  -- Do some stuff
end

function Test:Destroy() 
  -- Do some stuff
end

return Test

Now, at a basic level, I'd like to be able to associate these table definitions with an internal entity ID. So, I'd like somewhere to store a HashMap<Entity, Table>. On an update event, I'd like to loop through the tables and execute the appropriate function in the Lua context. The issue I'm running into currently is lifetimes. With the following astoundingly naive solution:

pub struct ScriptSystem<'a> {
    host: Lua,
    initialized: BitSet,
    behaviours: HashMap<Entity, Table<'a>>,
}

impl<'a> ScriptSystem<'a> {
    pub fn new() -> Self {
        ScriptSystem {
            host: Lua::new(),
            initialized: BitSet::new(),
            behaviours: HashMap::new(),
        }
    }
}

impl<'a, 'b> System<'a> for ScriptSystem<'b> {
    type SystemData = (
        Entities<'a>,
        ReadStorage<'a, Script>,
        Fetch<'a, AssetStorage<ScriptAsset>>,
        Fetch<'a, Time>,
    );

    fn run(&mut self, (entities, script, storage, time): Self::SystemData) {
        let behaviour = self.host.eval::<Table>(asset.source.as_str(), Some(asset.name.as_str()));
        self.behaviours.insert(entity, behaviour);
    }
}

I run into predictable lifetime errors, because the lifetime of the Table value returned from the eval() call would outlive the borrowed content self.host but must also adhere to the lifetime parameter 'b and so survive with the struct. I have tried every workaround I know of to fix this issue, including wrapping both the Lua instance and HashMap in a common struct with a shared lifetime. I think it's time to face the fact that I'm probably just doing something plain wrong, and was curious if there is a solution to this sort of use case using rlua.

@dkushner dkushner changed the title Storing table references alongside Lua. Storing Table references alongside Lua. Dec 5, 2017
@kyren
Copy link
Contributor

kyren commented Dec 6, 2017

So, I actually see two ways of solving this, one which would be a feature request to rlua, and one using the rust rental crate. I was originally writing this response to tell you about the rental crate, but while writing it I realized that there might be a case for adding a specific helpful feature to rlua.

Okay, first the solution involving the rental crate. The rental crate is designed to solve this exact sort of problem, It's a bit complicated to use, but once you get the hang of it, what it allows you to do is safely construct structs that "self borrow", if the thing they're borrowing has "pointer stability". In other words, you can have a struct like this:

struct LuaWithTables {
    lua: Box<Lua>,
    table: Table<Lua, 'lua>, // magic lifetime
}

If lua is kept in a Box, Rc, Arc, or something similar where the heap pointer is guaranteed to be stable.

This is the sort of thing that I find is generally avoidable in pure rust, but turns up a lot in the impedance mismatch of rust vs a dynamic language. It's best if you can structure your design to avoid it, but the rental crate is there for when you absolutely HAVE to do this. Spellbound actually has a single (complex) use of the rental crate in order to package a "query" object from our ECS and give it to Lua, so I do understand your issue.

Here's a quick (untested) example of how the struct declaration looks when you use the rental crate:


rental! {
    mod lua_with_table {
        use rlua::{Lua, Table};
        
        #[rental]
        pub struct LuaWithTable {
            lua: Rc<Lua>,
            table: Table<'lua>,
        }
    }
}

Like I said though, try to avoid this if you can, because using the rental crate is a bit.. insane. Nothing against the author of the rental crate mind you, it's actually amazing that they can pull it off safely, but it's kind of nuts to actually use it.

For me, this has mostly come up when giving things that need to self borrow rust things (Arcs, RwLocks, etc) TO Lua, but here it seems you're running into the opposite problem. There may actually be a simpler solution to this sort of thing that I could implement in rlua, which is the second solution I wanted to mention.

If there was a way to access part of the Lua registry from the Lua struct, another solution is to, instead of keeping actual rlua reference types around, simply keep references inside the registry. This is basically already how all rlua reference types already work, there's even an internal type called LuaRef which does exactly this, the difference though is that LuaRef is automatically managed so that when it is created, it increments a usage counter and when it's dropped it decrements that usage counter. This is actually the part of rlua reference types that requires them to borrow the parent Lua instance.

If there was a way to either opt out of that management, you could easily keep Lua registry keys around without having to mess with the rental crate, at the cost of having to manage the lifetimes of the references yourself.

Maybe even better is the ability to just place things in the registry backed by &str keys only? What you would then have is effectively a global HashMap<String, LuaValue> that is carried with Lua, inaccessible from scripts but accessible to anything that gets a Lua handle of the same underlying instance.

Do you think that either of those things would be useful? Which would be MORE useful to you? Would BOTH be useful enough to include?

Edit: one more thing I should add, if you start messing with the rental crate, or if I add the ability for manually managed registry keys, you can run into issues with the Lua garbage collector being unable to "see" into custom userdata types.

For example, say there was some manually managed registry key type, and you removed that entry from the registry on Drop for some custom UserData type. If you place that UserData type into Lua, and somehow manage to get that UserData type to be owned by the type in the registry, you now have a reference cycle that Lua will NEVER be able to garbage collect. There's no way in the Lua C API to tell Lua that a given userdata in turn holds x references to other Lua values, in fact they're implemented using the Lua registry precisely because the API just doesn't have this concept in it.

You can read more about this kind of issue in the discussion for #41, if you're curious.

@dkushner
Copy link
Author

dkushner commented Dec 6, 2017

@kyren: Wow! Thank you so much for this detailed and enlightening write-up. rental was one of the solutions I came across while trying to address this issue and I had a similar feeling of "wow that's pretty cool, but probably not correct." I may have to go this route in the short term, but both of the options mentioned sound like a marked improvement.

If I may clarify my understanding of the situation a bit before suggesting either way: this issue seems to revolve around consistency between Rust variable lifetimes and the Lua GC, correct? So the desire is to manage Rust-side references to Lua variables such that the Lua GC cannot collect Rust variable references whose lifetimes have not expired. With my incredibly basic understanding, and having read through the implementation of LuaRef, it seems like the issue is that each LuaRef needs to borrow the Lua instance itself in order to ensure that it cannot outlive its origin (I think this is the root of the my issue, as you have described).

If there was a way to either opt out of that management, you could easily keep Lua registry keys around without having to mess with the rental crate, at the cost of having to manage the lifetimes of the references yourself.

Would this not sacrifice some of the safety guarantees you were aiming for with this project? I'm all for usability but wouldn't want to detract from the value already being provided. As an opt-in unsafe feature that might work pretty well. I'm curious how you'd segregate that kind of thing and how deeply that would impact the current API (i.e. build flags or completely parallel struct/trait implementations)?

Maybe even better is the ability to just place things in the registry backed by &str keys only? What you would then have is effectively a global HashMap<String, LuaValue> that is carried with Lua, inaccessible from scripts but accessible to anything that gets a Lua handle of the same underlying instance.

This solution intrigues me quite a bit. Could you elaborate on what an implementation like this would look like?

@kyren
Copy link
Contributor

kyren commented Dec 7, 2017

If I may clarify my understanding of the situation a bit before suggesting either way: this issue seems to revolve around consistency between Rust variable lifetimes and the Lua GC, correct? So the desire is to manage Rust-side references to Lua variables such that the Lua GC cannot collect Rust variable references whose lifetimes have not expired. With my incredibly basic understanding, and having read through the implementation of LuaRef, it seems like the issue is that each LuaRef needs to borrow the Lua instance itself in order to ensure that it cannot outlive its origin (I think this is the root of the my issue, as you have described).

I.. think you have it basically right, but I think it might be simpler than you're thinking. None of these issues are specific to Rust or Rust lifetimes at all, they're intrinsic to how the Lua C API works. The primary way that C side variables (I'm saying C here because Lua is fundamentally a C API, this is true across all users of the Lua C API, so substitute whatever language is consuming Lua for C here) get passed to the Lua API is through the Lua stack. All operations use the Lua stack, when you call a function, you have to push the function, then the parameters of that function to the stack, the return values are then placed on the stack when the function returns, the API is fundamentally based around this stack.

So, say you want to create a Lua Table, and keep it around so that the C side can keep track of it, mutate it, pass it to multiple functions etc. One way of doing this would be to, using the Lua C API, first create a new table at the very bottom of the stack (index 1), and then just kind of.. leave it there, making sure never to accidentally remove it from the stack and knowing that it's at index 1 every time C needs to access it.

This is problematic for a number of reasons, but one big one is that it doesn't scale very well. If you need to keep track of 10 variables, you have to logcially.. preallocate an area of the stack for all variables that C needs to keep track of, and you end up just abusing the stack for something it was not meant to do. Also, when you're being called from Lua, the state pointer you get may actually be referring to a different coroutine (edit: or a protected stack in a pcall), and they have their own independent stacks, so you may not be able to get at variables that you want to be available universally to C.

For these reasons, there is a magic table that is ONLY accessible to C, which is called "the registry". You can place any Lua value into the registry, either with an automatic unique integer key, or with a string (or any lua value actually), and anywhere that C is being called, C can go get values out of the registry and put them in the stack or put values into the registry from the stack. It is a table of global variables available at all times to C. This is what rlua uses to manage rust side handles.

There's no automatic garbage collection that happens on the registry. It is actually very simple, it is a regular lua table that is just.. only accessible from C, that's all. When you place a value in the registry from C, that value will be there forever until C sets that value to nil.

So, for rlua (and selene, and other high level Lua APIs for other languages) uses this registry with automatic integer keys to implement "C (rust) side handles". When you get a handle, like Table, the internals of rlua is setting a value in the registry primarily so that it is accessible at all to rlua, because where else would it go? The registry is HOW you create arbitrary lifetime handles, there is no real other sensible way of doing it.

You can contrast this with hlua, which gives back "handles" into the Lua stack. This IS faster and sometimes closer to how you might use the C API in some cases, but only for a limited subset of cases. This is the reason that you can't have long lived handles in hlua, because the handles are simple stack indexes, any further manipulation of the stack may actually invalidate them, which means that the handles are severely limited, and tend to lock the Lua API in strange ways to maintain stack consistency.

So, the reason that LuaRef borrows Lua is simply to ensure that when a handle goes out of scope, the element in the registry is set to nil, that's all. You could easily implement a version of rlua that just returned handles and had no Drop implementation for LuaRef, and what would happen would be everything that you ever had a LuaRef to in Rust would just live in the registry forever, and it would be a horrible space leak, but you COULD do it.

All I'm suggesting is that there be (kind of) a version of LuaRef that makes the user responsible for knowing when something in the registry is no longer needed and setting it to nil. It would not cause any safety problems whatsoever, because once you get a handle back from the registry, secretly it would be using the OTHER registry scheme behind the scenes, and all live LuaRefs would still be pointing to valid registry entries, it would simply be a parallel way of getting at that global registry table.

The API would be brain dead simple, it could look like this:

let lua = Lua::new();
lua.set_registry("my_global", lua.create_table()?)?;
let table: Table = lua.get_registry("my_global")?;
// The table actually would have a reference in the registry *twice* at this point, one for 'my_global' and one for whatever the automatic integer key is for the table handle.
lua.set_registry("my_global", Nil)?;
// 'table' still works, the backing table still exists
drop(table);
// NOW the table is not in the registry at all, which means it is no longer accessible, and will eventually be garbage collected.

So, this kind of problem shows up no matter what language you're using, this is not intrinsic to Rust, but rather how the Lua C API actually works. The other problem I was mentioning was a "garbage collector" issue, but it's not an issue really from the Lua C API side, it's just something that you can do which ends up being problematic from how it works. Now, since we have these handles into the Lua registry, what happens if we place one of these handles into a userdata, and that userdata ends up in the registry? There is no way to say to the Lua C API that a value in the registry only exists because of some other value in the registry, that's not how the registry works. So, as soon as you start using the registry in this way, you run the risk of, for example, the following situation:

struct A {
    userdata: Option<Userdata>, // We're assuming that Userdata here has no lifetime, normally it does, but assume that you can do this for the sake of argument.
}

impl Userdata for A {}

Now, create an A, with userdata: None, and create a Lua::Userdata out of it. Then, take that handle, and set the userdata field to the value of that handle, and now you have a userdata in the registry that will never naturally go away. Since the only way that the element in the registry will be set to nil is from the Drop impl of userdata in A, it will NEVER go away because that Drop impl will never be called while a still exists in the registry.

What the Lua C API is missing is a way of creating long lived userdata and telling Lua about what references that userdata has without using the registry, and in other ways "participating" in the garbage collector system. Because of the way the lua registry works, that is just not something that's generally possible to do using the Lua C API, from any language.

It gets even worse, because how would you have a Userdata with no lifetime? One way would be, to keep a Userdata<'lua> along with an Rc, but now look what's happened. Now, the entire Lua instance will never be dropped, because you effectively have an Rc cycle of Lua, where an Rc in turn contains another Rc. THAT is what I was trying to warn you about.

What this means is that it's just generally NEVER a good idea to keep handles inside Userdata, and that was the discussion for issue #41. Instead, if there was some API in Lua to get at the registry in a way that acknowledged how the registry actually worked, with no automatic Drop, it would allow users of rlua to get around all of these messy lifetime problems in ways that were appropriate for their specific application.

For example, since the registry amounts to "C side global lua variables", maybe for your particular use case, if you need some kind of Table to always be accessible, just set that table to some string registry key. It may "leak", in that it will never automatically be set to nil, but maybe for your particular use case that kind of behavior is fine? This API would be a LOT easier to understand than trying to abuse rental and accidentally creating complicated LuaRef cycles, and because it would be easy to explain and easy to use, if you create a space leak by adding a lot of registry keys, it's at least simple to understand what's happening.

Does that make the current situation any clearer?

@dkushner
Copy link
Author

dkushner commented Dec 10, 2017

@kyren: Again, thank you so much for dedicating your time to such an incredibly detailed and thorough explanation. This is really above and beyond what I'd expect from any project maintainer, let alone one whose games I've enjoyed so much 😄.

This does make the issue far more clear, thank you. The API you outlined seems like it would be a very valuable feature, indeed. Would certainly make it easier to store and interact with Lua refs for the purposes discussed.

And message received loud and clear on the not storing references in UserData. 👍

EDIT: The more I consider this approach, the nicer it feels. What work needs to be done to move towards this? I'd be more than happy to contribute however I can, my knowledge of the domain being limited as it is.

@kyren
Copy link
Contributor

kyren commented Jan 27, 2018

I should let you know that there is a registry API available now, since 0.10.2. However, the released version of 0.10.2 had some bugs with the non-string registry API, that are fixed now in master and will be released soon.

@kyren
Copy link
Contributor

kyren commented Jan 27, 2018

Okay, 0.11.0 will be released shortly, and it's worth noting that this contains another API which you may find useful, which is the "user value" api for AnyUserData. Simply place any rlua::Value into a userdata, and that userdata will forever be associated with that value, retrievable from rust at any location. You could combine with UserData::add_function for a simple way to get an associated value for a userdata.

Edit: This is implemented using a Lua C API that I was until now unaware of, which is lua_getuservalue / lua_setuservalue. This API seem specifically designed to solve the problem outlined here where the Lua gc can't "see" into a userdata, so this solution has none of those problems and is much easier than dealing with a containing Table or something like that.

@dkushner
Copy link
Author

@kyren: Tremendous work, kyren! I'll give it a check ASAP.

@psychon
Copy link

psychon commented Feb 11, 2018

Sorry for capturing this issue, but I feel like the following best belongs here.

I just want to chime in and provide another use case for LuaRef that is currently not possible: Let's say I have some kind of graphical system. The rust code exports some kind of "Window" object to Lua. This has a visible property. What I would like to do is to prevent a Window from being garbage collected while it is visible (since otherwise I either have to decouple the lifetime of the actual window from its rlua wrapper in rust, or the window disappears at a random time when the GC decides to collect it).

I think I can implement the above by having a LuaRef to the window while it is visible. This references the window object from the registry and prevents it from being GCd.

Since you are worried about reference cycles:
The lack of a public LuaRef is currently worked around by WayCooler by basically creating a global table called __my_references and storing the references in there. This has the same downsides as exporting LuaRef would have, but with the additional downside of having this table accessible to regular Lua code.

(Note that the above example is made up; the actual code provides an API for registering callback functions and has to keep them alive somehow. However, the above felt like a better example to me and will actually have to be implemented at some point.)

So since it is already possible to shoot into your own shoot like this, would exporting LuaRef really at that much foot-shooting-potential? (Well, okay, it would make it easier to shoot into your own foot like this, so of course a big warning in the API documentation would still be needed.)

@kyren
Copy link
Contributor

kyren commented Feb 11, 2018

Have you taken a look at what the RegistryKey API in 0.12 offers? You can store RegistryKeys basically anywhere, and they're effectively equivalent to the LuaRef types, just with an extra speed bump. They're LuaRef without the cool deref on drop, and without a reference to the Lua state. You can manually garbage collect them though by calling Lua::expire_registry_values, or you can drop (EDIT: not drop, remove.. as in manually call Lua::remove_registry_value) them all manually.

You have full foot shooting potential with them, it's extremely easy to create reference cycles that Lua::expire_registry_values cannot solve.

Edit: Just to clarify, exporting LuaRef is not that useful, LuaRef is what causes all the lifetime issues in the first place because it borrows the parent Lua. Having a LuaRef is exactly the same as having a LuaValue, except you don't know what the type of the LuaRef is. RegistryKey is basicallyLuaRef but without the borrow, which is why there's an extra step to clean up the registry afterwards.

Also, I read through the (very long) discussion above, and it's kind of funny how out of date a lot of it is. There are now effectively THREE different ways to solve the original issue: RegistryKey, named registry values, and userdata "user values". Also, chucklefish no longer has to use the rental crate to manage passing locks to lua, becuase Lua::scope now exists.

I'm much, much happier with the API as of 0.12, and a large number of things that were once difficult or impossible with the API are now much more straightforward.

@psychon
Copy link

psychon commented Feb 11, 2018

Oh, thanks. I did indeed miss RegistryKey and it seems to be just what I'd need. Thanks!
(So does that mean this issue can now be closed?)

@kyren
Copy link
Contributor

kyren commented Feb 11, 2018

Yeah, it can. I feel a bit bad closing other people's issues, I'd rather let them decide it's resolved. That being said, there are about.. 4-5 open issues that are basically resolved right now.

@kyren
Copy link
Contributor

kyren commented Feb 28, 2018

I think this is pretty much resolved in several different ways now, I'm going to close this.

@kyren kyren closed this as completed Feb 28, 2018
@dkushner
Copy link
Author

For what its worth, I can confirm this was closed several times over by the excellent work done by @kyren. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants