From fe39ae09bfe59788934e5d86bc44caa18c62e581 Mon Sep 17 00:00:00 2001 From: Alex Orlenko Date: Tue, 11 May 2021 00:53:07 +0100 Subject: [PATCH] Try different approach for errors handling. Instead of convering error to CallbackError in error message handler, do it earlier at callback_error stage. Better fix for #44. --- src/ffi/shim/shim.c | 40 +++++++++++++++++++++++++--------------- src/util.rs | 44 +++++++++++++------------------------------- 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/src/ffi/shim/shim.c b/src/ffi/shim/shim.c index 2ed2ec7..c06b83a 100644 --- a/src/ffi/shim/shim.c +++ b/src/ffi/shim/shim.c @@ -32,8 +32,7 @@ const void *MLUA_WRAPPED_ERROR_KEY = NULL; const void *MLUA_WRAPPED_PANIC_KEY = NULL; extern void wrapped_error_traceback(lua_State *L, int error_idx, - int traceback_idx, - int convert_to_callback_error); + int traceback_idx); extern int mlua_hook_proc(lua_State *L, lua_Debug *ar); @@ -76,9 +75,12 @@ static int lua_call_rust(lua_State *L) { if (ret == -1 /* WrappedError */) { if (lua_checkstack(L, LUA_TRACEBACK_STACK) != 0) { luaL_traceback(L, L, NULL, 0); - // Attach traceback - wrapped_error_traceback(L, -2, -1, 0); + // Convert to CallbackError and attach traceback + wrapped_error_traceback(L, -2, -1); lua_pop(L, 1); + } else { + // Convert to CallbackError with error message as a traceback + wrapped_error_traceback(L, -1, 0); } } lua_error(L); @@ -92,7 +94,18 @@ void lua_call_mlua_hook_proc(lua_State *L, lua_Debug *ar) { lua_newuserdata(L, max(MLUA_WRAPPED_ERROR_SIZE, MLUA_WRAPPED_PANIC_SIZE)); lua_rotate(L, 1, 1); int ret = mlua_hook_proc(L, ar); - if (ret == -1) { + if (ret < 0) { + if (ret == -1 /* WrappedError */) { + if (lua_checkstack(L, LUA_TRACEBACK_STACK) != 0) { + luaL_traceback(L, L, NULL, 0); + // Convert to CallbackError and attach traceback + wrapped_error_traceback(L, -2, -1); + lua_pop(L, 1); + } else { + // Convert to CallbackError with error message as a traceback + wrapped_error_traceback(L, -1, 0); + } + } lua_error(L); } } @@ -420,19 +433,16 @@ int error_traceback(lua_State *state) { return 1; } - if (is_wrapped_struct(state, -1, MLUA_WRAPPED_ERROR_KEY) != 0) { - // Convert to CallbackError - wrapped_error_traceback(state, -1, 0, 1); + if (MLUA_WRAPPED_PANIC_KEY == NULL || + is_wrapped_struct(state, -1, MLUA_WRAPPED_PANIC_KEY) || + is_wrapped_struct(state, -1, MLUA_WRAPPED_ERROR_KEY)) { return 1; } - if (MLUA_WRAPPED_PANIC_KEY != NULL && - !is_wrapped_struct(state, -1, MLUA_WRAPPED_PANIC_KEY)) { - const char *s = luaL_tolstring(state, -1, NULL); - if (lua_checkstack(state, LUA_TRACEBACK_STACK) != 0) { - luaL_traceback(state, state, s, 1); - lua_remove(state, -2); - } + const char *s = luaL_tolstring(state, -1, NULL); + if (lua_checkstack(state, LUA_TRACEBACK_STACK) != 0) { + luaL_traceback(state, state, s, 1); + lua_remove(state, -2); } return 1; diff --git a/src/util.rs b/src/util.rs index e6ba759..27edb21 100644 --- a/src/util.rs +++ b/src/util.rs @@ -313,7 +313,7 @@ where Ok(Err(err)) => { ffi::lua_settop(state, 1); let error_ud = ffi::lua_touserdata(state, 1); - ptr::write(error_ud as *mut WrappedError, WrappedError(err, None)); + ptr::write(error_ud as *mut WrappedError, WrappedError(err)); get_gc_metatable_for::(state); ffi::lua_setmetatable(state, -2); -1 @@ -338,23 +338,17 @@ pub unsafe extern "C" fn wrapped_error_traceback( state: *mut ffi::lua_State, error_idx: c_int, traceback_idx: c_int, - convert_to_callback_error: c_int, ) { let wrapped_error = mlua_expect!( get_gc_userdata::(state, error_idx).as_mut(), "cannot get " ); - - if convert_to_callback_error != 0 { - let traceback = wrapped_error - .1 - .take() - .unwrap_or_else(|| "".into()); - let cause = Arc::new(wrapped_error.0.clone()); - wrapped_error.0 = Error::CallbackError { traceback, cause }; - } else { - wrapped_error.1 = Some(to_string(state, traceback_idx)); - } + let traceback = match traceback_idx { + 0 => "".to_string(), + _ => to_string(state, traceback_idx), + }; + let cause = Arc::new(wrapped_error.0.clone()); + wrapped_error.0 = Error::CallbackError { traceback, cause }; } // Returns Lua main thread for Lua >= 5.2 or checks that the passed thread is main for Lua 5.1. @@ -384,7 +378,7 @@ pub unsafe fn get_main_state(state: *mut ffi::lua_State) -> Option<*mut ffi::lua // Uses 2 stack spaces and does not call checkstack. pub unsafe fn push_wrapped_error(state: *mut ffi::lua_State, err: Error) -> Result<()> { let error_ud = ffi::safe::lua_newwrappederror(state)? as *mut WrappedError; - ptr::write(error_ud, WrappedError(err, None)); + ptr::write(error_ud, WrappedError(err)); get_gc_metatable_for::(state); ffi::lua_setmetatable(state, -2); Ok(()) @@ -457,8 +451,7 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) -> Result<(*const callback_error(state, |_| { check_stack(state, 3)?; - let wrapped_error = get_gc_userdata::(state, -1).as_ref(); - let err_buf = if let Some(error) = wrapped_error { + let err_buf = if let Some(error) = get_wrapped_error(state, -1).as_ref() { let err_buf_key = &ERROR_PRINT_BUFFER_KEY as *const u8 as *const c_void; ffi::lua_rawgetp(state, ffi::LUA_REGISTRYINDEX, err_buf_key); let err_buf = ffi::lua_touserdata(state, -1) as *mut String; @@ -468,20 +461,9 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) -> Result<(*const // Depending on how the API is used and what error types scripts are given, it may // be possible to make this consume arbitrary amounts of memory (for example, some // kind of recursive error structure?) - let _ = write!(&mut (*err_buf), "{}", error.0); - match error { - WrappedError(Error::CallbackError { .. }, _) => {} - // This is a workaround to avoid having duplicate traceback for string errors - // with attached traceback at the `error_traceback` (message handler) stage. - WrappedError(Error::RuntimeError(ref err), _) - if err.contains("\nstack traceback:\n") => {} - WrappedError(_, Some(ref traceback)) => { - let _ = write!(&mut (*err_buf), "\n{}", traceback); - } - _ => {} - } + let _ = write!(&mut (*err_buf), "{}", error); // Find first two sources that caused the error - let mut source1 = error.0.source(); + let mut source1 = error.source(); let mut source0 = source1.and_then(|s| s.source()); while let Some(source) = source0.and_then(|s| s.source()) { source1 = source0; @@ -557,7 +539,7 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) -> Result<(*const callback_error(state, |_| { check_stack(state, 2)?; let error_ud = ffi::safe::lua_newwrappederror(state)? as *mut WrappedError; - ptr::write(error_ud, WrappedError(Error::CallbackDestructed, None)); + ptr::write(error_ud, WrappedError(Error::CallbackDestructed)); get_gc_metatable_for::(state); ffi::lua_setmetatable(state, -2); Ok(-1) // to trigger lua_error @@ -621,7 +603,7 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) -> Result<(*const Ok((wrapped_error_key, wrapped_panic_key)) } -pub(crate) struct WrappedError(pub Error, pub Option); +pub(crate) struct WrappedError(pub Error); pub(crate) struct WrappedPanic(pub Option>); // Converts the given lua value to a string in a reasonable format without causing a Lua error or