Clean up/improve some scriptapi error handling code

7 files changed, 103 insertions, 90 deletions

diff --git a/src/script/common/c_internal.cpp b/src/script/common/c_internal.cpp
index 66f6a9b98..df82dba14 100644
--- a/src/script/common/c_internal.cpp
+++ b/src/script/common/c_internal.cpp
@@ -101,42 +101,6 @@ void script_error(lua_State *L, int pcall_result, const char *mod, const char *f
 	throw LuaError(err_msg);
 }
 
-// Push the list of callbacks (a lua table).
-// Then push nargs arguments.
-// Then call this function, which
-// - runs the callbacks
-// - replaces the table and arguments with the return value,
-//     computed depending on mode
-void script_run_callbacks_f(lua_State *L, int nargs,
-	RunCallbacksMode mode, const char *fxn)
-{
-	FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments");
-
-	// Insert error handler
-	PUSH_ERROR_HANDLER(L);
-	int error_handler = lua_gettop(L) - nargs - 1;
-	lua_insert(L, error_handler);
-
-	// Insert run_callbacks between error handler and table
-	lua_getglobal(L, "core");
-	lua_getfield(L, -1, "run_callbacks");
-	lua_remove(L, -2);
-	lua_insert(L, error_handler + 1);
-
-	// Insert mode after table
-	lua_pushnumber(L, (int) mode);
-	lua_insert(L, error_handler + 3);
-
-	// Stack now looks like this:
-	// ... <error handler> <run_callbacks> <table> <mode> <arg#1> <arg#2> ... <arg#n>
-
-	int result = lua_pcall(L, nargs + 2, 1, error_handler);
-	if (result != 0)
-		script_error(L, result, NULL, fxn);
-
-	lua_remove(L, error_handler);
-}
-
 static void script_log_add_source(lua_State *L, std::string &message, int stack_depth)
 {
 	lua_Debug ar;
diff --git a/src/script/common/c_internal.h b/src/script/common/c_internal.h
index 4ddbed232..ab2d7b975 100644
--- a/src/script/common/c_internal.h
+++ b/src/script/common/c_internal.h
@@ -75,9 +75,6 @@ extern "C" {
 	}                                                   \
 }
 
-#define script_run_callbacks(L, nargs, mode) \
-	script_run_callbacks_f((L), (nargs), (mode), __FUNCTION__)
-
 // What script_run_callbacks does with the return values of callbacks.
 // Regardless of the mode, if only one callback is defined,
 // its return value is the total return value.
@@ -108,16 +105,17 @@ enum RunCallbacksMode
 	// are converted by readParam<bool> to true or false, respectively.
 };
 
+// Gets a backtrace of the current execution point
 std::string script_get_backtrace(lua_State *L);
+// Wrapper for CFunction calls that converts C++ exceptions to Lua errors
 int script_exception_wrapper(lua_State *L, lua_CFunction f);
+// Takes an error from lua_pcall and throws it as a LuaError
 void script_error(lua_State *L, int pcall_result, const char *mod, const char *fxn);
-void script_run_callbacks_f(lua_State *L, int nargs,
-	RunCallbacksMode mode, const char *fxn);
 
 bool script_log_unique(lua_State *L, std::string message, std::ostream &log_to,
 	int stack_depth = 1);
 
-enum class DeprecatedHandlingMode {
+enum DeprecatedHandlingMode {
 	Ignore,
 	Log,
 	Error
diff --git a/src/script/cpp_api/s_base.h b/src/script/cpp_api/s_base.h
index 7a8ebc85a..06df2abe3 100644
--- a/src/script/cpp_api/s_base.h
+++ b/src/script/cpp_api/s_base.h
@@ -128,8 +128,11 @@ protected:
 	lua_State* getStack()
 		{ return m_luastack; }
 
+	// Checks that stack size is sane
 	void realityCheck();
+	// Takes an error from lua_pcall and throws it as a LuaError
 	void scriptError(int result, const char *fxn);
+	// Dumps stack contents for debugging
 	void stackDump(std::ostream &o);
 
 	void setGameDef(IGameDef* gamedef) { m_gamedef = gamedef; }
diff --git a/src/script/cpp_api/s_client.cpp b/src/script/cpp_api/s_client.cpp
index f2cc9730b..c889fffa0 100644
--- a/src/script/cpp_api/s_client.cpp
+++ b/src/script/cpp_api/s_client.cpp
@@ -33,7 +33,11 @@ void ScriptApiClient::on_mods_loaded()
 	lua_getglobal(L, "core");
 	lua_getfield(L, -1, "registered_on_mods_loaded");
 	// Call callbacks
-	runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	try {
+		runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 void ScriptApiClient::on_shutdown()
@@ -44,7 +48,11 @@ void ScriptApiClient::on_shutdown()
 	lua_getglobal(L, "core");
 	lua_getfield(L, -1, "registered_on_shutdown");
 	// Call callbacks
-	runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	try {
+		runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 bool ScriptApiClient::on_sending_message(const std::string &message)
@@ -56,7 +64,12 @@ bool ScriptApiClient::on_sending_message(const std::string &message)
 	lua_getfield(L, -1, "registered_on_sending_chat_message");
 	// Call callbacks
 	lua_pushstring(L, message.c_str());
-	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	try {
+		runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
@@ -69,7 +82,12 @@ bool ScriptApiClient::on_receiving_message(const std::string &message)
 	lua_getfield(L, -1, "registered_on_receiving_chat_message");
 	// Call callbacks
 	lua_pushstring(L, message.c_str());
-	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	try {
+		runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
@@ -82,7 +100,11 @@ void ScriptApiClient::on_damage_taken(int32_t damage_amount)
 	lua_getfield(L, -1, "registered_on_damage_taken");
 	// Call callbacks
 	lua_pushinteger(L, damage_amount);
-	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	try {
+		runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 void ScriptApiClient::on_hp_modification(int32_t newhp)
@@ -94,7 +116,11 @@ void ScriptApiClient::on_hp_modification(int32_t newhp)
 	lua_getfield(L, -1, "registered_on_hp_modification");
 	// Call callbacks
 	lua_pushinteger(L, newhp);
-	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	try {
+		runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 void ScriptApiClient::on_death()
@@ -105,7 +131,11 @@ void ScriptApiClient::on_death()
 	lua_getglobal(L, "core");
 	lua_getfield(L, -1, "registered_on_death");
 	// Call callbacks
-	runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	try {
+		runCallbacks(0, RUN_CALLBACKS_MODE_FIRST);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 void ScriptApiClient::environment_step(float dtime)
@@ -120,8 +150,7 @@ void ScriptApiClient::environment_step(float dtime)
 	try {
 		runCallbacks(1, RUN_CALLBACKS_MODE_FIRST);
 	} catch (LuaError &e) {
-		getClient()->setFatalError(std::string("Client environment_step: ") + e.what() + "\n"
-				+ script_get_backtrace(L));
+		getClient()->setFatalError(e);
 	}
 }
 
@@ -146,7 +175,11 @@ void ScriptApiClient::on_formspec_input(const std::string &formname,
 		lua_pushlstring(L, value.c_str(), value.size());
 		lua_settable(L, -3);
 	}
-	runCallbacks(2, RUN_CALLBACKS_MODE_OR_SC);
+	try {
+		runCallbacks(2, RUN_CALLBACKS_MODE_OR_SC);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+	}
 }
 
 bool ScriptApiClient::on_dignode(v3s16 p, MapNode node)
@@ -164,7 +197,12 @@ bool ScriptApiClient::on_dignode(v3s16 p, MapNode node)
 	pushnode(L, node, ndef);
 
 	// Call functions
-	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	try {
+		runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return lua_toboolean(L, -1);
 }
 
@@ -183,7 +221,12 @@ bool ScriptApiClient::on_punchnode(v3s16 p, MapNode node)
 	pushnode(L, node, ndef);
 
 	// Call functions
-	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	try {
+		runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
@@ -200,7 +243,12 @@ bool ScriptApiClient::on_placenode(const PointedThing &pointed, const ItemDefini
 	push_item_definition(L, item);
 
 	// Call functions
-	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	try {
+		runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
@@ -217,7 +265,12 @@ bool ScriptApiClient::on_item_use(const ItemStack &item, const PointedThing &poi
 	push_pointed_thing(L, pointed, true);
 
 	// Call functions
-	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	try {
+		runCallbacks(2, RUN_CALLBACKS_MODE_OR);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
@@ -238,7 +291,12 @@ bool ScriptApiClient::on_inventory_open(Inventory *inventory)
 		lua_rawset(L, -3);
 	}
 
-	runCallbacks(1, RUN_CALLBACKS_MODE_OR);
+	try {
+		runCallbacks(1, RUN_CALLBACKS_MODE_OR);
+	} catch (LuaError &e) {
+		getClient()->setFatalError(e);
+		return true;
+	}
 	return readParam<bool>(L, -1);
 }
 
diff --git a/src/script/cpp_api/s_env.cpp b/src/script/cpp_api/s_env.cpp
index c11de3757..874c37b6e 100644
--- a/src/script/cpp_api/s_env.cpp
+++ b/src/script/cpp_api/s_env.cpp
@@ -53,13 +53,7 @@ void ScriptApiEnv::environment_Step(float dtime)
 	lua_getfield(L, -1, "registered_globalsteps");
 	// Call callbacks
 	lua_pushnumber(L, dtime);
-	try {
-		runCallbacks(1, RUN_CALLBACKS_MODE_FIRST);
-	} catch (LuaError &e) {
-		getServer()->setAsyncFatalError(
-				std::string("environment_Step: ") + e.what() + "\n"
-				+ script_get_backtrace(L));
-	}
+	runCallbacks(1, RUN_CALLBACKS_MODE_FIRST);
 }
 
 void ScriptApiEnv::player_event(ServerActiveObject *player, const std::string &type)
@@ -76,13 +70,7 @@ void ScriptApiEnv::player_event(ServerActiveObject *player, const std::string &t
 	// Call callbacks
 	objectrefGetOrCreate(L, player);   // player
 	lua_pushstring(L,type.c_str()); // event type
-	try {
-		runCallbacks(2, RUN_CALLBACKS_MODE_FIRST);
-	} catch (LuaError &e) {
-		getServer()->setAsyncFatalError(
-				std::string("player_event: ") + e.what() + "\n"
-				+ script_get_backtrace(L) );
-	}
+	runCallbacks(2, RUN_CALLBACKS_MODE_FIRST);
 }
 
 void ScriptApiEnv::initializeEnvironment(ServerEnvironment *env)
@@ -257,9 +245,8 @@ void ScriptApiEnv::on_emerge_area_completion(
 	try {
 		PCALL_RES(lua_pcall(L, 4, 0, error_handler));
 	} catch (LuaError &e) {
-		server->setAsyncFatalError(
-				std::string("on_emerge_area_completion: ") + e.what() + "\n"
-				+ script_get_backtrace(L));
+		// Note: don't throw here, we still need to run the cleanup code below
+		server->setAsyncFatalError(e);
 	}
 
 	lua_pop(L, 1); // Pop error handler
@@ -300,4 +287,4 @@ void ScriptApiEnv::on_liquid_transformed(
 	}
 
 	runCallbacks(2, RUN_CALLBACKS_MODE_FIRST);
-}
\ No newline at end of file
+}
diff --git a/src/script/cpp_api/s_item.cpp b/src/script/cpp_api/s_item.cpp
index 24955cefc..48dce14f3 100644
--- a/src/script/cpp_api/s_item.cpp
+++ b/src/script/cpp_api/s_item.cpp
@@ -29,6 +29,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "inventory.h"
 #include "inventorymanager.h"
 
+#define WRAP_LUAERROR(e, detail) \
+	LuaError(std::string(__FUNCTION__) + ": " + (e).what() + ". " detail)
+
 bool ScriptApiItem::item_OnDrop(ItemStack &item,
 		ServerActiveObject *dropper, v3f pos)
 {
@@ -49,7 +52,7 @@ bool ScriptApiItem::item_OnDrop(ItemStack &item,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
@@ -81,7 +84,7 @@ bool ScriptApiItem::item_OnPlace(ItemStack &item,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
@@ -108,7 +111,7 @@ bool ScriptApiItem::item_OnUse(ItemStack &item,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
@@ -133,7 +136,7 @@ bool ScriptApiItem::item_OnSecondaryUse(ItemStack &item,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
@@ -165,7 +168,7 @@ bool ScriptApiItem::item_OnCraft(ItemStack &item, ServerActiveObject *user,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
@@ -197,7 +200,7 @@ bool ScriptApiItem::item_CraftPredict(ItemStack &item, ServerActiveObject *user,
 		try {
 			item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
-			throw LuaError(std::string(e.what()) + ". item=" + item.name);
+			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
 	}
 	lua_pop(L, 2);  // Pop item and error handler
diff --git a/src/script/cpp_api/s_nodemeta.cpp b/src/script/cpp_api/s_nodemeta.cpp
index c081e9fc4..7ab3757f3 100644
--- a/src/script/cpp_api/s_nodemeta.cpp
+++ b/src/script/cpp_api/s_nodemeta.cpp
@@ -43,7 +43,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowMove(
 		return 0;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "allow_metadata_inventory_move", &ma.to_inv.p))
 		return count;
 
@@ -58,7 +58,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowMove(
 	PCALL_RES(lua_pcall(L, 7, 1, error_handler));
 	if (!lua_isnumber(L, -1))
 		throw LuaError("allow_metadata_inventory_move should"
-				" return a number, guilty node: " + nodename);
+				" return a number. node=" + nodename);
 	int num = luaL_checkinteger(L, -1);
 	lua_pop(L, 2); // Pop integer and error handler
 	return num;
@@ -81,7 +81,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowPut(
 		return 0;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "allow_metadata_inventory_put", &ma.to_inv.p))
 		return stack.count;
 
@@ -94,7 +94,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowPut(
 	PCALL_RES(lua_pcall(L, 5, 1, error_handler));
 	if(!lua_isnumber(L, -1))
 		throw LuaError("allow_metadata_inventory_put should"
-				" return a number, guilty node: " + nodename);
+				" return a number. node=" + nodename);
 	int num = luaL_checkinteger(L, -1);
 	lua_pop(L, 2); // Pop integer and error handler
 	return num;
@@ -117,7 +117,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowTake(
 		return 0;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "allow_metadata_inventory_take", &ma.from_inv.p))
 		return stack.count;
 
@@ -130,7 +130,7 @@ int ScriptApiNodemeta::nodemeta_inventory_AllowTake(
 	PCALL_RES(lua_pcall(L, 5, 1, error_handler));
 	if (!lua_isnumber(L, -1))
 		throw LuaError("allow_metadata_inventory_take should"
-				" return a number, guilty node: " + nodename);
+				" return a number. node=" + nodename);
 	int num = luaL_checkinteger(L, -1);
 	lua_pop(L, 2); // Pop integer and error handler
 	return num;
@@ -153,7 +153,7 @@ void ScriptApiNodemeta::nodemeta_inventory_OnMove(
 		return;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "on_metadata_inventory_move", &ma.from_inv.p))
 		return;
 
@@ -186,7 +186,7 @@ void ScriptApiNodemeta::nodemeta_inventory_OnPut(
 		return;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "on_metadata_inventory_put", &ma.to_inv.p))
 		return;
 
@@ -217,7 +217,7 @@ void ScriptApiNodemeta::nodemeta_inventory_OnTake(
 		return;
 
 	// Push callback function on stack
-	std::string nodename = ndef->get(node).name;
+	const auto &nodename = ndef->get(node).name;
 	if (!getItemCallback(nodename.c_str(), "on_metadata_inventory_take", &ma.from_inv.p))
 		return;