mlua-rs / mlua

High level Lua 5.4/5.3/5.2/5.1 (including LuaJIT) and Roblox Luau bindings to Rust with async/await support
Other
1.73k stars 138 forks source link

bug: `c_void` not a Sync #413

Closed cppcoffee closed 5 months ago

cppcoffee commented 5 months ago

The bug demo is as follows:

Cargo.toml:

[dependencies]
mlua = { version = "0.9.8", features = ["lua54", "vendored", "async", "send"] }
tokio = { version = "1.38.0", features = ["full"] }

src/main.rs:

use mlua::{Lua, Result};

#[tokio::main]
async fn main() -> Result<()> {
    let s = "local a = 1";

    let boxed = Box::new(Box::pin(async move {
        let lua = Lua::new();
        lua.load(s).exec_async().await.unwrap();
    }));

    tokio::spawn(async move {
        boxed.await;
    });

    Ok(())
}
cppcoffee commented 5 months ago

We'll split this into three PRs to resolve this issue.

The following are the three PRs, each enabling state_main, LightUserData, and Thread to support Send/Sync.

https://github.com/mlua-rs/mlua/pull/412 https://github.com/mlua-rs/mlua/pull/414 https://github.com/mlua-rs/mlua/pull/415

khvzak commented 5 months ago

It's not a bug, it's by design.

I'm afraid the PRs you raised are unsound. Just an example how to cause a segfault (using your changes):

let lua = Lua::new();

std::thread::scope(|s| {
    s.spawn(|| {
        loop {
            lua.load("return 1+1").exec().unwrap();
        }
    });
    s.spawn(|| {
        loop {
            lua.load("return 1+1").exec().unwrap();
        }
    });
});

Lua is not thread safe and applying Sync trait without significantly rewriting mlua core is not possible.

I hope next version will support proper sync feature with all protection measure in place, but it's not trivial task at all.

cppcoffee commented 5 months ago

Would it be better to introduce a LuaGuard and use mutex locks to execute lua?