aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorkwolekr <kwolekr@minetest.net>2015-10-31 16:31:43 -0400
committerkwolekr <kwolekr@minetest.net>2015-11-01 11:32:05 -0500
commit52e5b513ed9dc143c967c733423fe751e1b663d1 (patch)
tree524462147e28889f655cb2ae867ebb412d9c75e3
parentd198e420ec54be47fe2285b3205953282ec06742 (diff)
downloadminetest-52e5b513ed9dc143c967c733423fe751e1b663d1.tar.gz
minetest-52e5b513ed9dc143c967c733423fe751e1b663d1.tar.bz2
minetest-52e5b513ed9dc143c967c733423fe751e1b663d1.zip
Fix Lua scripting synchronization
For several years now, the lua script lock has been completely broken. This commit fixes the main issue (creation of a temporary rather than scoped object), and fixes a subsequent deadlock issue caused by nested script API calls by adding support for recursive mutexes.
-rw-r--r--src/script/cpp_api/s_base.cpp10
-rw-r--r--src/script/cpp_api/s_base.h4
-rw-r--r--src/script/cpp_api/s_internal.h42
-rw-r--r--src/threading/mutex.cpp16
-rw-r--r--src/threading/mutex.h2
5 files changed, 57 insertions, 17 deletions
diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp
index b40d31533..71369e3d7 100644
--- a/src/script/cpp_api/s_base.cpp
+++ b/src/script/cpp_api/s_base.cpp
@@ -67,10 +67,11 @@ public:
ScriptApiBase
*/
-ScriptApiBase::ScriptApiBase()
+ScriptApiBase::ScriptApiBase() :
+ m_luastackmutex(true)
{
#ifdef SCRIPTAPI_LOCK_DEBUG
- m_locked = false;
+ m_lock_recursion_count = 0;
#endif
m_luastack = luaL_newstate();
@@ -157,9 +158,14 @@ void ScriptApiBase::loadScript(const std::string &script_path)
// - runs the callbacks
// - replaces the table and arguments with the return value,
// computed depending on mode
+// This function must only be called with scriptlock held (i.e. inside of a
+// code block with SCRIPTAPI_PRECHECKHEADER declared)
void ScriptApiBase::runCallbacksRaw(int nargs,
RunCallbacksMode mode, const char *fxn)
{
+#ifdef SCRIPTAPI_LOCK_DEBUG
+ assert(m_lock_recursion_count > 0);
+#endif
lua_State *L = getStack();
FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments");
diff --git a/src/script/cpp_api/s_base.h b/src/script/cpp_api/s_base.h
index 20f4bc11b..d30373ce1 100644
--- a/src/script/cpp_api/s_base.h
+++ b/src/script/cpp_api/s_base.h
@@ -28,6 +28,7 @@ extern "C" {
}
#include "irrlichttypes.h"
+#include "threads.h"
#include "threading/mutex.h"
#include "threading/mutex_auto_lock.h"
#include "common/c_types.h"
@@ -111,7 +112,8 @@ protected:
std::string m_last_run_mod;
bool m_secure;
#ifdef SCRIPTAPI_LOCK_DEBUG
- bool m_locked;
+ int m_lock_recursion_count;
+ threadid_t m_owning_thread;
#endif
private:
diff --git a/src/script/cpp_api/s_internal.h b/src/script/cpp_api/s_internal.h
index 7daf8c03f..651fed95f 100644
--- a/src/script/cpp_api/s_internal.h
+++ b/src/script/cpp_api/s_internal.h
@@ -32,28 +32,50 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#ifdef SCRIPTAPI_LOCK_DEBUG
#include "debug.h" // assert()
+
class LockChecker {
public:
- LockChecker(bool *variable) {
- assert(*variable == false);
+ LockChecker(int *recursion_counter, threadid_t *owning_thread)
+ {
+ m_lock_recursion_counter = recursion_counter;
+ m_owning_thread = owning_thread;
+ m_original_level = *recursion_counter;
+
+ if (*m_lock_recursion_counter > 0)
+ assert(thr_is_current_thread(*m_owning_thread));
+ else
+ *m_owning_thread = thr_get_current_thread_id();
- m_variable = variable;
- *m_variable = true;
+ (*m_lock_recursion_counter)++;
}
- ~LockChecker() {
- *m_variable = false;
+
+ ~LockChecker()
+ {
+ assert(thr_is_current_thread(*m_owning_thread));
+ assert(*m_lock_recursion_counter > 0);
+
+ (*m_lock_recursion_counter)--;
+
+ assert(*m_lock_recursion_counter == m_original_level);
}
+
private:
- bool *m_variable;
+ int *m_lock_recursion_counter;
+ int m_original_level;
+ threadid_t *m_owning_thread;
};
-#define SCRIPTAPI_LOCK_CHECK LockChecker(&(this->m_locked))
+#define SCRIPTAPI_LOCK_CHECK \
+ LockChecker scriptlock_checker( \
+ &this->m_lock_recursion_count, \
+ &this->m_owning_thread)
+
#else
-#define SCRIPTAPI_LOCK_CHECK while(0)
+ #define SCRIPTAPI_LOCK_CHECK while(0)
#endif
#define SCRIPTAPI_PRECHECKHEADER \
- MutexAutoLock(this->m_luastackmutex); \
+ MutexAutoLock scriptlock(this->m_luastackmutex); \
SCRIPTAPI_LOCK_CHECK; \
realityCheck(); \
lua_State *L = getStack(); \
diff --git a/src/threading/mutex.cpp b/src/threading/mutex.cpp
index eb1c7d61d..e12b79185 100644
--- a/src/threading/mutex.cpp
+++ b/src/threading/mutex.cpp
@@ -34,15 +34,25 @@ DEALINGS IN THE SOFTWARE.
#define UNUSED(expr) do { (void)(expr); } while (0)
-
-Mutex::Mutex()
+Mutex::Mutex(bool recursive)
{
#ifdef _WIN32
+ // Windows critical sections are recursive by default
+ UNUSED(recursive);
+
InitializeCriticalSection(&mutex);
#else
- int ret = pthread_mutex_init(&mutex, NULL);
+ pthread_mutexattr_t attr;
+ pthread_mutexattr_init(&attr);
+
+ if (recursive)
+ pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
+
+ int ret = pthread_mutex_init(&mutex, &attr);
assert(!ret);
UNUSED(ret);
+
+ pthread_mutexattr_destroy(&attr);
#endif
}
diff --git a/src/threading/mutex.h b/src/threading/mutex.h
index f1a4882b7..62a482787 100644
--- a/src/threading/mutex.h
+++ b/src/threading/mutex.h
@@ -49,7 +49,7 @@ DEALINGS IN THE SOFTWARE.
class Mutex
{
public:
- Mutex();
+ Mutex(bool recursive=false);
~Mutex();
void lock();
void unlock();