Skip to content

Commit

Permalink
Use dynamic Lua state ownership instead of compile-time module cfg …
Browse files Browse the repository at this point in the history
…flag to allow

creating new VMs in module mode (and destructing them properly).
  • Loading branch information
khvzak committed Aug 31, 2024
1 parent 1634c43 commit 825bdbf
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 17 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ $ lua5.4 -e 'require("my_module").hello("world")'
hello, world!
```

On macOS, you need to set additional linker arguments. One option is to compile with `cargo rustc --release -- -C link-arg=-undefined -C link-arg=dynamic_lookup`, the other is to create a `.cargo/config` with the following content:
On macOS, you need to set additional linker arguments. One option is to compile with `cargo rustc --release -- -C link-arg=-undefined -C link-arg=dynamic_lookup`, the other is to create a `.cargo/config.toml` with the following content:
``` toml
[target.x86_64-apple-darwin]
rustflags = [
Expand Down
2 changes: 1 addition & 1 deletion src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ impl Lua {
#[inline]
pub unsafe fn init_from_ptr(state: *mut ffi::lua_State) -> Lua {
Lua {
raw: RawLua::init_from_ptr(state),
raw: RawLua::init_from_ptr(state, false),
collect_garbage: true,
}
}
Expand Down
14 changes: 8 additions & 6 deletions src/state/extra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const REF_STACK_RESERVE: c_int = 1;
pub(crate) struct ExtraData {
pub(super) lua: MaybeUninit<Lua>,
pub(super) weak: MaybeUninit<WeakLua>,
pub(super) owned: bool,

pub(super) registered_userdata: FxHashMap<TypeId, c_int>,
pub(super) registered_userdata_mt: FxHashMap<*const c_void, Option<TypeId>>,
Expand All @@ -46,7 +47,7 @@ pub(crate) struct ExtraData {

pub(super) safe: bool,
pub(super) libs: StdLib,
#[cfg(feature = "module")]
// Used in module mode
pub(super) skip_memory_check: bool,

// Auxiliary thread to store references
Expand Down Expand Up @@ -88,8 +89,9 @@ pub(crate) struct ExtraData {
impl Drop for ExtraData {
fn drop(&mut self) {
unsafe {
#[cfg(feature = "module")]
self.lua.assume_init_drop();
if !self.owned {
self.lua.assume_init_drop();
}

self.weak.assume_init_drop();
}
Expand All @@ -111,7 +113,7 @@ impl ExtraData {
#[cfg(any(feature = "lua51", feature = "luajit", feature = "luau"))]
pub(super) const ERROR_TRACEBACK_IDX: c_int = 1;

pub(super) unsafe fn init(state: *mut ffi::lua_State) -> XRc<UnsafeCell<Self>> {
pub(super) unsafe fn init(state: *mut ffi::lua_State, owned: bool) -> XRc<UnsafeCell<Self>> {
// Create ref stack thread and place it in the registry to prevent it
// from being garbage collected.
let ref_thread = mlua_expect!(
Expand Down Expand Up @@ -141,14 +143,14 @@ impl ExtraData {
let extra = XRc::new(UnsafeCell::new(ExtraData {
lua: MaybeUninit::uninit(),
weak: MaybeUninit::uninit(),
owned,
registered_userdata: FxHashMap::default(),
registered_userdata_mt: FxHashMap::default(),
last_checked_userdata_mt: (ptr::null(), None),
registry_unref_list: Arc::new(Mutex::new(Some(Vec::new()))),
app_data: AppData::default(),
safe: false,
libs: StdLib::NONE,
#[cfg(feature = "module")]
skip_memory_check: false,
ref_thread,
// We need some reserved stack space to move values in and out of the ref stack.
Expand Down Expand Up @@ -188,7 +190,7 @@ impl ExtraData {
raw: XRc::clone(raw),
collect_garbage: false,
});
if cfg!(not(feature = "module")) {
if self.owned {
XRc::decrement_strong_count(XRc::as_ptr(raw));
}
self.weak.write(WeakLua(XRc::downgrade(raw)));
Expand Down
14 changes: 7 additions & 7 deletions src/state/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,13 @@ pub struct RawLua {
pub(super) extra: XRc<UnsafeCell<ExtraData>>,
}

#[cfg(not(feature = "module"))]
impl Drop for RawLua {
fn drop(&mut self) {
unsafe {
if !(*self.extra.get()).owned {
return;
}

let mem_state = MemoryState::get(self.main_state);

ffi::lua_close(self.main_state);
Expand Down Expand Up @@ -115,7 +118,7 @@ impl RawLua {
ffi::luau_codegen_create(state);
}

let rawlua = Self::init_from_ptr(state);
let rawlua = Self::init_from_ptr(state, true);
let extra = rawlua.lock().extra.get();

mlua_expect!(
Expand Down Expand Up @@ -154,7 +157,7 @@ impl RawLua {
rawlua
}

pub(super) unsafe fn init_from_ptr(state: *mut ffi::lua_State) -> XRc<ReentrantMutex<Self>> {
pub(super) unsafe fn init_from_ptr(state: *mut ffi::lua_State, owned: bool) -> XRc<ReentrantMutex<Self>> {
assert!(!state.is_null(), "Lua state is NULL");
if let Some(lua) = Self::try_from_ptr(state) {
return lua;
Expand Down Expand Up @@ -191,7 +194,7 @@ impl RawLua {
);

// Init ExtraData
let extra = ExtraData::init(main_state);
let extra = ExtraData::init(main_state, owned);

// Register `DestructedUserdata` type
get_destructed_userdata_metatable(main_state);
Expand Down Expand Up @@ -704,10 +707,7 @@ impl RawLua {
// MemoryInfo is empty in module mode so we cannot predict memory limits
match MemoryState::get(self.main_state) {
mem_state if !mem_state.is_null() => (*mem_state).memory_limit() == 0,
#[cfg(feature = "module")]
_ => (*self.extra.get()).skip_memory_check, // Check the special flag (only for module mode)
#[cfg(not(feature = "module"))]
_ => false,
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ fn test_c_function() -> Result<()> {
let lua = Lua::new();

unsafe extern "C-unwind" fn c_function(state: *mut mlua::lua_State) -> std::os::raw::c_int {
let lua = Lua::init_from_ptr(state);
lua.globals().set("c_function", true).unwrap();
ffi::lua_pushboolean(state, 1);
ffi::lua_setglobal(state, b"c_function\0" as *const _ as *const _);
0
}

Expand Down
File renamed without changes.
12 changes: 12 additions & 0 deletions tests/module/loader/tests/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,18 @@ fn test_module_multi_from_thread() -> Result<()> {
.exec()
}

#[test]
fn test_module_new_vm() -> Result<()> {
let lua = make_lua()?;
lua.load(
r#"
local mod = require("test_module.new_vm")
assert(mod.eval("return \"hello, world\"") == "hello, world")
"#,
)
.exec()
}

fn make_lua() -> Result<Lua> {
let (dylib_path, dylib_ext, separator);
if cfg!(target_os = "macos") {
Expand Down
12 changes: 12 additions & 0 deletions tests/module/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ fn test_module2(lua: &Lua) -> LuaResult<LuaTable> {
Ok(exports)
}

#[mlua::lua_module]
fn test_module_new_vm(lua: &Lua) -> LuaResult<LuaTable> {
let eval = lua.create_function(|_, prog: String| {
let lua = Lua::new();
lua.load(prog).eval::<Option<String>>()
})?;

let exports = lua.create_table()?;
exports.set("eval", eval)?;
Ok(exports)
}

#[mlua::lua_module]
fn test_module_error(_: &Lua) -> LuaResult<LuaTable> {
Err("custom module error".into_lua_err())
Expand Down

1 comment on commit 825bdbf

@eigeen
Copy link

@eigeen eigeen commented on 825bdbf Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently encountered this issue, and just as I was about to submit a new issue, I saw that this feature has already been added in the new version of the source code. Thank you for your work; I will use it as soon as the new version is released.

Before this, if I needed to borrow a lua_State object from FFI, I could only do so through unsafe FFI functions or by wrapping it in mlua::Lua and leaking it after use, which could potentially lead to memory leaks.

Please sign in to comment.