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.

diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp
index b40d31533caf5ec5426aef6c4902b081736737ee..71369e3d75ce0924297f02e5fefbfc548e88d2d4 100644 (file)
--- 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 20f4bc11b0fe88344ca30c188696b13b69aab6d1..d30373ce1520a25a20cb7abc16b4b1ec4ff7f64d 100644 (file)
--- 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 7daf8c03f02c60c2195239c7e40be9d9ff7df13a..651fed95f4c609de503dcc68e34f2855836b6dee 100644 (file)
--- 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 eb1c7d61d65b8493c64fe25e9ea6b6ae5a46f302..e12b791851bf289aa1f109cab25baa645f2e54b0 100644 (file)
--- 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 f1a4882b742f1b1bf83e032eb636d7ae52c9571c..62a48278717a3706a1fbf972c30292f4ce635c9d 100644 (file)
--- 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();