Why is Lua not Send?

[mkpankov]

I believe wrapping Lua in Arc<Mutex<...>> should allow passing it to other threads and synchronized access?

Right now it's this:

error[E0277]: the trait bound `*mut rlua::ffi::lua_State: std::marker::Send` is not satisfied in `rlua::Lua`
  --> src/main.rs:29:29
   |
29 |     let event_loop_thread = thread::spawn({
   |                             ^^^^^^^^^^^^^ `*mut rlua::ffi::lua_State` cannot be sent between threads safely
   |
   = help: within `rlua::Lua`, the trait `std::marker::Send` is not implemented for `*mut rlua::ffi::lua_State`
   = note: required because it appears within the type `rlua::Lua`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Mutex<rlua::Lua>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<std::sync::Mutex<rlua::Lua>>`
   = note: required because it appears within the type `[closure@src/main.rs:32:9: 55:10 lua:std::sync::Arc<std::sync::Mutex<rlua::Lua>>, barrier:std::sync::Arc<std::sync::Barrier>]`
   = note: required by `std::thread::spawn`

[jonas-schievink]

Yeah, this should be safe to do. Just needs an impl.

[kyren]

Unfortunately, if you do that, you have to universally decide that all userdata inside Lua should also be Send, which is really bad if you want to put an Rc into Lua. Since there's no way to abstract over whether Lua should be all Send or all !Send, I picked !Send intentionally. Perhaps there is some way of abstracting this, or maybe macroing this, so you can choose whether you want the Send version of Lua or the !Send version?

Barring that, I guess I should ask which is the least annoying version, Lua + all userdata being Send, or !Send. I guess you could always just use Arc instead of Rc, but if you need it to be Send you REALLY need it to be Send right?

[jonas-schievink]

Oh, right. That's a bit unfortunate. I guess you could try to create the Lua inside the thread if you pass everything needed for construction of your environment, but perhaps that's not always possible?

Anyways, I think we should look at a few use cases where Lua: Send makes an important difference.

[kyren]

I actually have a solution to this inside the project I'm currently working on that creates a thread JUST for Lua in certain cases where otherwise it would need to be Send, it's extremely unfortunate. In fact, I have an incredible hack of a wrapper just for it:

https://gist.github.com/kyren/d010f25cc6d98bcfc4044c44518b5676

Lua implementing Send would prevent me from having to do this obviously. In my case, I have a set of systems that run on an entity component store, and they're sent to a thread pool for execution, and of course I can't actually do that because Lua is not Send. Unfortunately x2, I also use Rc in userdata inside Lua, but of course this could probably be changed to Arc. For my use cases having Lua be Send is actually preferable, but it felt wrong to introduce a Send requirement on userdata for something as thread unsafe as Lua, and I figured that I was the unusual case.

[kyren]

You know, I guess this could be a config option, it's not as bad as I'm thinking because you can use the multiple trait alias hack (https://stackoverflow.com/questions/26070559/type-alias-for-multiple-traits) and only have the config option once, I was thinking I would have to add cfg entries to like 20 things. Is that as easy as I'm thinking it is?

Edit: Also, is it bad style to make something like that controllable from a feature flag?

[kyren]

I experimented with a branch that adds Send bounds to userdata, and controls whether Error is Send + Sync. The tl;dr here is that I now think !Send is less onerous than Send, which is the reverse of what I suspected earlier.

Trying to integrate it into the current Chucklefish project was pretty enlightening. I'm fairly confident that Send bounds on userdata would actually be very annoying depending on what you want to do with rlua, to the extent of possibly being a deal breaker for some of our use cases. Since this is for use by Chucklefish, we are of course not going to seppuku and modify rlua so we can't use it anymore BUT I might still be on board with adding a cfg option. The problem is that we use rlua with userdata that contains things like RwLockReadGuard. We actually do self borrows to hold the RwLockReadGuard using the rental crate, but you would run into the same problem if you held RwLockReadGuard to 'static variables or similar, and of course you'd have the same problem with Mutexes and RefCell.

Okay, so maybe we would just not use the 'send' cfg option that's fair, but we DO still need the Error types to be Send, which means that the internally held external errors need to be Send + Sync (so Arc is Send). I guess that's fair as well, we could just not touch the current error types, and leave the current state of the code as what you would get without the 'send' cfg option.

Does that sound logical? Is it weird to have this sort of thing as a cfg option, and is it weird that not enabling send still leaves you with Send + Sync bounds on errors? Is it worth it to have two settings?

I was hoping this would fix the situation for Chucklefish somewhat, but since it looks like that's not possible, I'd like to see if there's a really clear use case here and see what you guys think before making a PR. If I remove the Error changes, it's actually a fairly simple change, I just think that making it a cfg option is pretty gross and I'd like to avoid this unless it's really useful to somebody.

[UserAB1236872]

You could make a SendableLua struct that implements Send and can be treated as a Lua.

impl Lua {
    /// Allows you to send `Lua` contexts over threads.
    ///
    /// Note that if you have registered 
    /// types which have shared interior mutability (`Rc` for instance) into
    /// `Lua`, and handles still exist outside the context
    /// this may cause race conditions if you do not use proper
    /// locking mechanisms. 
    ///
    /// For plain data, or interior mutability where no reference
    /// exists outside the `Lua` context itself, this should be
    /// completely safe.
    pub unsafe fn to_sendable(self) -> SendableLua { 
        SendableLua { lua: self }
    }
}

Where SendableLua is just

/// Allows sending Lua contexts across threads.
/// Please see `Lua::to_sendable` for details on
/// safety and usage of this functionality.
pub struct SendableLua {
    lua: Lua
}

unsafe impl Send for SendableLua {}

impl Deref for SendableLua {
    type Target = Lua;
    impl deref(&self) -> &Self::Target {
         &self.lua
     }
}

impl DerefMut for SendableLua {
    impl deref_mut(&mut self) -> &mut Self::Target {
        &mut self.lua
    }
}

This allows people to opt-in to sending while understanding that depending on exactly what they fed into Lua it may not be strictly safe.

[kyren]

I think, if you're willing to be unsafe about it, it's actually much simpler for the user of rlua to just make their own struct SendableLua(Lua) and unsafe impl Send for SendableLua. I don't think that has to go in our API. In fact, if you do it that way it can be much safer (though not completely safe), because you can make sure that no !Send types leak out of whatever containing type you have. If you make sure not to leak !Send types, it actually IS safe except for thread local storage, so that would probably be a better recommended usage pattern than having a blessed Send wrapper type that definitely does leak !Send types.

[dkushner]

Having tried @LinearZoetrope's suggestion above, the compiler still will not allow me to share a Lua instance in a multithreaded context. I receive:

error[E0277]: the trait bound `*mut rlua::ffi::lua_State: std::marker::Sync` is not satisfied in `scripting::script::SharedLua`
  --> src/scripting/bundle.rs:23:14
   |
23 |             .add(ScriptSystem::new(), "script_system", &["script_processor"]))
   |              ^^^ `*mut rlua::ffi::lua_State` cannot be shared between threads safely
   |
   = help: within `scripting::script::SharedLua`, the trait `std::marker::Sync` is not implemented for `*mut rlua::ffi::lua_State`
   = note: required because it appears within the type `rlua::Lua`
   = note: required because it appears within the type `scripting::script::SharedLua`
   = note: required because of the requirements on the impl of `std::marker::Send` for `&scripting::script::SharedLua`
   = note: required because it appears within the type `scripting::script::ScriptSystem<'_>`

My implementation of the suggested solution looked like this, for reference:

pub struct SharedLua(Lua);

unsafe impl Send for SharedLua { }

impl Deref for SharedLua {
    type Target = Lua;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for SharedLua {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

It is entirely possible I've just missed something stupid and simple, but was unable to get this working.

[Timidger]

@dkushner It's complaining that SharedLua is not Sync because Lua is not sync (therefore it cannot inherit the trait).

You could unsafe impl Sync for SharedLua {} but I'm not sure that's wise.

[kyren]

= note: required because of the requirements on the impl of std::marker::Send for &scripting::script::SharedLua

Ahh, that doesn't seem right, it seems like you're trying to send a reference to a Lua across threads. That's EXTREMELY dangerous, Lua is not just !Sync "in theory", it is WILDLY !Sync. It's also very much !Send, but it's at least feasible to Send it without causing UB by being very careful about not having any dangling references when you Send it, and not using TLS.

I'm not sure what you're trying to do exactly, but if you put SharedLua into a Mutex, and then made some kind of interface in SharedLua that had two very important guarantees:

1. No references leak outside of the interface, so don't give back references to userdata or components inside userdata, or Rc, or anything that is !Send, because everything inside Lua is still morally !Send.
2. No use of thread local storage

Then it MIGHT be "safe". I'm not sure what the correct word to use there is, but I mean, you might be able to guarantee that using SharedLua cannot cause UB.

[dkushner]

@Timidger, ah sorry I excluded that in my snippet but I did indeed try explicitly implementing Sync for SharedLua, same essential issue.

@kyren, gotcha. I was already sort of tenuous on completely tossing away any guarantees just to get it to satisfy the trait bounds of my ECS, so I'll try and find an alternate solution as you suggest. This limitation, of course, makes perfect sense for all of the reasons you describe.

[torkleyy]

It's also very much !Send, but it's at least feasible to Send it without causing UB by being very careful about not having any dangling references when you Send it, and not using TLS.

Are there internals that would make it !Send or is this only about the user data?

[jonas-schievink]

Are there internals that would make it !Send or is this only about the user data?

Only userdata, IIRC.

We could "just" introduce a SendableLua that requires userdata to also be Send, if we built the right abstraction for it. Then you would effectively have 2 ways to create a Lua state, one requiring userdata to be Send and being Send itself, and the current Lua that is neither Send nor requires Sendable userdata.

This could also be done using a sort of empty marker type parameter of Lua so we don't actually end up with 2 distinct Lua types (I think this pattern is semi-regularly used, but I don't know if this is the right solution here).

[jonas-schievink]

To clarify, I mean something like this:

use std::marker::PhantomData;

trait Userdata {}   // not important here

struct Lua<S> {
    _phantom: PhantomData<S>,
}

enum Sendable {}
enum NotSendable {}

unsafe impl Send for Lua<Sendable> {}

impl Lua<Sendable> {
    fn take_userdata<T: Userdata + Send>(&self, userdata: T) {}
}

impl Lua<NotSendable> {
    fn take_userdata<T: Userdata>(&self, userdata: T) {}
}

This would need to be done everywhere a userdata can be stored in the Lua state, so perhaps it's too much clutter.