Added mutex tests (#1501)

* Added mutex tests. Add errorcheck in safe mode for Posix threads. Make non-recursive locks fail when used recursively on Windows. Fix thread pool tests. Simple locking count.

---------

Co-authored-by: Christoffer Lerno <christoffer@aegik.com>

lib/std/core/runtime.c3

@@ -221,6 +221,7 @@ fn bool run_tests(TestUnit[] tests)
 		name.appendf("Testing %s ", unit.name);
 		name.append_repeat('.', max_name - unit.name.len + 2);
 		io::printf("%s ", name.str_view());
+		(void)io::stdout().flush();
 		if (libc::setjmp(&context.buf) == 0)
 		{
 			if (catch err = unit.func())

lib/std/os/posix/threads.c3

@@ -3,8 +3,8 @@ import std::thread;
 import libc;
 
 const PTHREAD_MUTEX_NORMAL = 0;
-const PTHREAD_MUTEX_ERRORCHECK = 1;
-const PTHREAD_MUTEX_RECURSIVE = 2;
+const PTHREAD_MUTEX_ERRORCHECK = env::LINUX ? 2 : 1;
+const PTHREAD_MUTEX_RECURSIVE = env::LINUX ? 1 : 2;
 
 def PosixThreadFn = fn void*(void*);
 distinct Pthread_t = void*;

lib/std/threads/os/thread_posix.c3

@@ -24,6 +24,12 @@ fn void! NativeMutex.init(&self, MutexType type)
 	{
 		if (posix::pthread_mutexattr_settype(&attr, posix::PTHREAD_MUTEX_RECURSIVE)) return ThreadFault.INIT_FAILED?;
 	}
+	else
+	{
+		$if env::COMPILER_SAFE_MODE:
+			if (posix::pthread_mutexattr_settype(&attr, posix::PTHREAD_MUTEX_ERRORCHECK)) return ThreadFault.INIT_FAILED?;
+		$endif
+	}
 	if (posix::pthread_mutex_init(&self.mutex, &attr)) return ThreadFault.INIT_FAILED?;
 	self.initialized = true;
 }

lib/std/threads/os/thread_win32.c3

@@ -12,7 +12,7 @@ struct NativeMutex
 	}
 	// Size is less than a Win32_HANDLE so due to alignment
 	// there is no benefit to pack these into a bitstruct.
-	bool already_locked;
+	uint locks;
 	bool recursive;
 	bool timed;
 }
@@ -40,7 +40,7 @@ struct NativeConditionVariable
 
 fn void! NativeMutex.init(&mtx, MutexType type)
 {
-	mtx.already_locked = false;
+	mtx.locks = 0;
 	mtx.recursive = (bool)(type & thread::MUTEX_RECURSIVE);
 	mtx.timed = (bool)(type & thread::MUTEX_TIMED);
 	if (!mtx.timed)
@@ -53,6 +53,7 @@ fn void! NativeMutex.init(&mtx, MutexType type)
 
 fn void! NativeMutex.destroy(&mtx)
 {
+	mtx.locks = 0;
 	if (!mtx.timed)
 	{
 		win32::deleteCriticalSection(&mtx.critical_section);
@@ -79,11 +80,11 @@ fn void! NativeMutex.lock(&mtx)
 
 		}
 	}
-	if (!mtx.recursive)
+	if (!mtx.recursive && mtx.locks)
 	{
-		while (mtx.already_locked) win32::sleep(1);
+		return ThreadFault.LOCK_FAILED?;
 	}
-	mtx.already_locked = true;
+	mtx.locks++;
 }
 
 
@@ -103,20 +104,11 @@ fn void! NativeMutex.lock_timeout(&mtx, ulong ms)
 		default:
 			return ThreadFault.LOCK_FAILED?;
 	}
-	if (!mtx.recursive)
+	if (!mtx.recursive && mtx.locks)
 	{
-		usz left_timeout = ms - 1;
-		while (mtx.already_locked)
-		{
-			if (left_timeout-- == 0)
-			{
-				win32::releaseMutex(mtx.handle);
-				return ThreadFault.LOCK_TIMEOUT?;
-			}
-			win32::sleep(1);
-		}
-		mtx.already_locked = true;
+		return ThreadFault.LOCK_FAILED?;
 	}
+	mtx.locks++;
 }
 
 fn bool NativeMutex.try_lock(&mtx)
@@ -128,7 +120,7 @@ fn bool NativeMutex.try_lock(&mtx)
 	if (!success) return false;
 	if (!mtx.recursive)
 	{
-		if (mtx.already_locked)
+		if (mtx.locks)
 		{
 			if (mtx.timed)
 			{
@@ -140,14 +132,15 @@ fn bool NativeMutex.try_lock(&mtx)
 			}
 			return false;
 		}
-		mtx.already_locked = true;
 	}
+	mtx.locks++;
 	return true;
 }
 
 fn void! NativeMutex.unlock(&mtx)
 {
-	mtx.already_locked = false;
+	if (!mtx.locks) return ThreadFault.UNLOCK_FAILED?;
+	mtx.locks--;
 	if (!mtx.timed)
 	{
 		win32::leaveCriticalSection(&mtx.critical_section);

releasenotes.md

@@ -66,6 +66,8 @@
 - Improved error messages on `Foo a = foo { 1 };` #1496
 - Bug in json decoder escape handling.
 - Fix bug when reading zip manifest, that would not return a zero terminated string. #1490
+- Fix thread tests.
+- Detect recursion errors on non-recursive mutexes in safe mode.
 
 ### Stdlib changes
 - Additional init functions for hashmap.

test/unit/stdlib/threads/mutex.c3

@@ -0,0 +1,148 @@
+module thread_test;
+
+import std::thread;
+import std::os;
+
+const TEST_MAGNITUDE = 10;
+
+
+fn void! lock_control_test() @test
+{
+    Mutex m;
+    m.init()!;
+    m.lock()!;
+    assert(@catch(m.lock()));
+}
+
+fn void! unlock_control_test() @test
+{
+    Mutex m;
+    m.init()!;
+    assert(@catch(m.unlock()));
+}
+
+fn void! lock_with_double_unlock_test() @test
+{
+    Mutex m;
+    m.init()!;
+
+    m.lock()!;
+    m.unlock()!;
+    assert(@catch(m.unlock()));
+}
+
+fn void! own_mutex(Mutex* m)
+{
+    m.lock()!!;
+    m.unlock()!!;
+}
+
+fn void! ensure_owner_checks() @test
+{
+    Mutex m;
+    m.init()!;
+
+    Thread[3 * TEST_MAGNITUDE] threads;
+    
+    foreach(&t : threads)
+    {
+        t.create((ThreadFn)&own_mutex, &m)!;
+    }
+
+    foreach(&t : threads)
+    {
+        t.join()!;
+    }
+
+    own_mutex(&m)!;
+}
+
+struct ArgsWrapper1
+{
+    Mutex* m;
+    ulong* v;
+}
+
+fn void shared_mutex_increment(ArgsWrapper1* args)
+{
+    args.m.lock()!!;
+    args.v++;
+    args.m.unlock()!!;
+}
+
+fn void shared_mutex_decrement(ArgsWrapper1* args)
+{
+    args.m.lock()!!;
+    args.v--;
+    args.m.unlock()!!;
+}
+
+fn void! shared_mutex() @test
+{
+    Mutex m;
+    m.init()!;
+    m.lock()!;
+
+    ulong v;
+
+    ArgsWrapper1 args =
+    {
+        .m = &m,
+        .v = &v
+    };
+
+    // An even number of threads must be chosen
+    Thread[6 * TEST_MAGNITUDE] threads;
+    for (int i = 0; i < threads.len / 2; i++)
+    {
+        (&threads[i]).create((ThreadFn)&shared_mutex_increment, &args)!;
+    }
+    for (int i = (threads.len / 2); i < threads.len; i++)
+    {
+        (&threads[i]).create((ThreadFn)&shared_mutex_decrement, &args)!;
+    }
+
+    m.unlock()!;
+    foreach(&t : threads)
+    {
+        t.join()!;
+    }
+    assert(v == 0);
+}
+
+// Recursive mutex
+
+fn void! acquire_recursively(RecursiveMutex* m)
+{
+    // TODO: The recursive mutex functions can not directly be called via pointer
+
+    for (usz i = 0; i < 5 * TEST_MAGNITUDE; i++)
+    {
+        ((Mutex*)m).lock()!!;
+    }
+
+    for (usz i = 0; i < 5 * TEST_MAGNITUDE; i++)
+    {
+        ((Mutex*)m).unlock()!!;
+    }
+}
+
+fn void! test_recursive_mutex() @test
+{
+    RecursiveMutex m;
+    m.init()!;
+    defer m.destroy()!!;
+
+    Thread[3 * TEST_MAGNITUDE] threads;
+    foreach(&t : threads)
+    {
+        t.create((ThreadFn)&acquire_recursively, &m)!;
+    }
+
+    foreach(&t : threads)
+    {
+        t.join()!;
+    }
+
+    return acquire_recursively(&m);
+}

test/unit/stdlib/threads/pool.c3

@@ -17,7 +17,7 @@ fn void! init_destroy() @test
 
 fn void! push_destroy() @test
 {
-	for (usz i = 0; i < 20; i++)
+	for FOO: (usz i = 0; i < 20; i++)
 	{
 		x = 0;
 		int y = 20;
@@ -26,9 +26,19 @@ fn void! push_destroy() @test
 		defer pool.destroy()!!;
 		work_done.lock()!!;
 		pool.push(&do_work, &y)!;
-		work_done.lock()!!;
-		assert(x == y, "%d: %d != %d", i, x, y);
 		work_done.unlock()!!;
+		for (int j = 0; j < 1000; j++)
+		{
+			work_done.lock()!!;
+			if (@atomic_load(x) == @atomic_load(y))
+			{
+				(void)work_done.unlock();
+				break FOO;
+			}
+			(void)work_done.unlock();
+			thread::yield();
+		}
+		assert(false, "y never changed");
 	}
 }
 
@@ -42,6 +52,7 @@ fn void! push_stop() @test
 		pool.init()!;
 		work_done.lock()!!;
 		pool.push(&do_work, &y)!;
+		work_done.unlock()!!;
 		pool.stop_and_destroy()!!;
 		assert(x == y, "%d: %d != %d", i, x, y);
 	}
@@ -56,11 +67,12 @@ fn void startup() @init {
 }
 
 fn void shutdown() @finalizer {
-	work_done.destroy()!!;
+	(void)work_done.destroy();
 }
 
 fn int do_work(void* arg)
 {
+	work_done.lock()!!;
 	x = *(int*)arg;
 	work_done.unlock()!!;
 	return 0;