From 6bb24ac773e2e70c2d85a515ad8dccec9c87b915 Mon Sep 17 00:00:00 2001 From: Y0SH1M4S73R Date: Thu, 1 Sep 2022 05:30:59 -0400 Subject: [PATCH] Fixes hard-dels related to lua signal handlers (#69555) When I changed the syntax of SS13.lua to account for the ability to properly index tables with datums, it turns out that the callbacks created for signal handlers and timeouts had circular references, resulting in hard-deletes. My first solution was to make it so signal handler and timeout callbacks use weakrefs, which get resolved in lua_state/call_function, but it turns out that, when calling the signal cleanup function, a qdeleted-but-not-yet-garbage-collected datum's weakref resolves to null because datum.gc_collected is set to GC_CURRENTLY_BEING_QDELETED before COMSIG_PARENT_QDELETING gets sent. To resolve this issue, Potato and Oranges both recommended that I make a snowflake variant of resolve which ignores whether the datum a weakref points to is qdeleted - only that it is null or it's weakref isn't the very weakref resolve was called on. This proc was given a lengthy autodoc comment describing when it should or shouldn't be used, to ensure it is only used in cases similar to the one I needed to create it for (needing to resolve a weakref to a datum in a COMSIG_PARENT_QDELETING handler registered on that very datum). --- code/datums/weakrefs.dm | 17 +++++++++++++++++ code/modules/admin/verbs/lua/lua_state.dm | 18 +++++++++++++++--- lua/SS13.lua | 6 +++--- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/code/datums/weakrefs.dm b/code/datums/weakrefs.dm index 9fb7f3b7cdd..27bd5d94330 100644 --- a/code/datums/weakrefs.dm +++ b/code/datums/weakrefs.dm @@ -76,6 +76,23 @@ var/datum/D = locate(reference) return (!QDELETED(D) && D.weak_reference == src) ? D : null +/** + * SERIOUSLY READ THE AUTODOC COMMENT FOR THIS PROC BEFORE EVEN THINKING ABOUT USING IT + * + * Like resolve, but doesn't care if the datum is being qdeleted but hasn't been deleted yet. + * + * The return value of this proc leaves hanging references if the datum is being qdeleted but hasn't been deleted yet. + * + * Do not do anything that would create a lasting reference to the return value, such as giving it a tag, putting it on the map, + * adding it to an atom's contents or vis_contents, giving it a key (if it's a mob), attaching it to an atom (if it's an image), + * or assigning it to a datum or list referenced somewhere other than a temporary value. + * + * Unless you're resolving a weakref to a datum in a COMSIG_PARENT_QDELETING signal handler registered on that very same datum, + * just use resolve instead. + */ +/datum/weakref/proc/hard_resolve() + var/datum/D = locate(reference) + return (D?.weak_reference == src) ? D : null /datum/weakref/vv_get_dropdown() . = ..() diff --git a/code/modules/admin/verbs/lua/lua_state.dm b/code/modules/admin/verbs/lua/lua_state.dm index d7151e226ca..a647a4ae587 100644 --- a/code/modules/admin/verbs/lua/lua_state.dm +++ b/code/modules/admin/verbs/lua/lua_state.dm @@ -88,6 +88,18 @@ GLOBAL_PROTECT(lua_usr) /datum/lua_state/proc/call_function(function, ...) var/call_args = length(args) > 1 ? args.Copy(2) : list() + if(islist(function)) + var/list/new_function_path = list() + for(var/path_element in function) + if(isweakref(path_element)) + var/datum/weakref/weak_ref = path_element + var/resolved = weak_ref.hard_resolve() + if(!resolved) + return list("status" = "errored", "param" = "Weakref in function path ([weak_ref] \ref[weak_ref]) resolved to null.", "name" = jointext(function, ".")) + new_function_path += resolved + else + new_function_path += path_element + function = new_function_path var/msg = "[key_name(usr)] called the lua function \"[function]\" with arguments: [english_list(call_args)]" log_lua(msg) @@ -99,7 +111,7 @@ GLOBAL_PROTECT(lua_usr) GLOB.lua_usr = tmp_usr if(isnull(result)) - result = list("status" = "errored", "param" = "__lua_call returned null (it may have runtimed - check the runtime logs)", "name" = "input") + result = list("status" = "errored", "param" = "__lua_call returned null (it may have runtimed - check the runtime logs)", "name" = islist(function) ? jointext(function, ".") : function) if(istext(result)) result = list("status" = "errored", "param" = result, "name" = islist(function) ? jointext(function, ".") : function) check_if_slept(result) @@ -118,7 +130,7 @@ GLOBAL_PROTECT(lua_usr) GLOB.IsLuaCall = FALSE if(isnull(result)) - result = list("status" = "errored", "param" = "__lua_awaken returned null (it may have runtimed - check the runtime logs)", "name" = "input") + result = list("status" = "errored", "param" = "__lua_awaken returned null (it may have runtimed - check the runtime logs)", "name" = "An attempted awaken") if(istext(result)) result = list("status" = "errored", "param" = result, "name" = "An attempted awaken") check_if_slept(result) @@ -135,7 +147,7 @@ GLOBAL_PROTECT(lua_usr) GLOB.IsLuaCall = FALSE if(isnull(result)) - result = list("status" = "errored", "param" = "__lua_resume returned null (it may have runtimed - check the runtime logs)", "name" = "input") + result = list("status" = "errored", "param" = "__lua_resume returned null (it may have runtimed - check the runtime logs)", "name" = "An attempted resume") if(istext(result)) result = list("status" = "errored", "param" = result, "name" = "An attempted resume") check_if_slept(result) diff --git a/lua/SS13.lua b/lua/SS13.lua index 3eef19407e9..d9c208edfeb 100644 --- a/lua/SS13.lua +++ b/lua/SS13.lua @@ -65,10 +65,10 @@ function SS13.register_signal(datum, signal, func, make_easy_clear_function) end local callback = SS13.new("/datum/callback", SS13.state, "call_function_return_first") callback:call_proc("RegisterSignal", datum, signal, "Invoke") - local path = { "SS13", "signal_handlers", datum, signal, callback, "func" } + local path = { "SS13", "signal_handlers", dm.global_proc("WEAKREF", datum), signal, dm.global_proc("WEAKREF", callback), "func" } callback.vars.arguments = { path } if not SS13.signal_handlers[datum]["_cleanup"] then - local cleanup_path = { "SS13", "signal_handlers", datum, "_cleanup", "func" } + local cleanup_path = { "SS13", "signal_handlers", dm.global_proc("WEAKREF", datum), "_cleanup", "func" } local cleanup_callback = SS13.new("/datum/callback", SS13.state, "call_function_return_first", cleanup_path) cleanup_callback:call_proc("RegisterSignal", datum, "parent_qdeleting", "Invoke") SS13.signal_handlers[datum]["_cleanup"] = { @@ -180,7 +180,7 @@ function SS13.set_timeout(time, func, timer) dm.global_proc("qdel", callback) func() end - local path = { "SS13", "timeouts", callback } + local path = { "SS13", "timeouts", dm.global_proc("WEAKREF", callback) } callback.vars.arguments = { path } end