From ffa5bd5c5899475becb51e8a1fa8985aa939bc4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:00:20 +0100 Subject: [PATCH 1/6] Add a trap to detect invalid uses of InterruptibleTaskMutex in conjunction with synchronized. --- source/vibe/core/sync.d | 51 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/source/vibe/core/sync.d b/source/vibe/core/sync.d index 20b50d6..2710ae0 100644 --- a/source/vibe/core/sync.d +++ b/source/vibe/core/sync.d @@ -274,7 +274,6 @@ final class LocalTaskSemaphore */ final class TaskMutex : core.sync.mutex.Mutex, Lockable { @safe: - private TaskMutexImpl!false m_impl; this(Object o) { m_impl.setup(); super(o); } @@ -376,7 +375,13 @@ final class InterruptibleTaskMutex : Lockable { private TaskMutexImpl!true m_impl; - this() { m_impl.setup(); } + this() + { + m_impl.setup(); + + // detects invalid usage within synchronized(...) + () @trusted { this.__monitor = cast(void*)&NoUseMonitor.instance(); } (); + } bool tryLock() nothrow { return m_impl.tryLock(); } void lock() { m_impl.lock(); } @@ -437,10 +442,15 @@ unittest { */ final class InterruptibleRecursiveTaskMutex : Lockable { @safe: - private RecursiveTaskMutexImpl!true m_impl; - this() { m_impl.setup(); } + this() + { + m_impl.setup(); + + // detects invalid usage within synchronized(...) + () @trusted { this.__monitor = cast(void*)&NoUseMonitor.instance(); } (); + } bool tryLock() { return m_impl.tryLock(); } void lock() { m_impl.lock(); } @@ -453,6 +463,39 @@ unittest { } +// Helper class to ensure that the non Object.Monitor compatible interruptible +// mutex classes are not accidentally used with the `synchronized` statement +private final class NoUseMonitor : Object.Monitor { + private static shared Proxy st_instance; + + static struct Proxy { + Object.Monitor monitor; + } + + static @property ref shared(Proxy) instance() + @safe nothrow { + static shared(Proxy)* inst = null; + if (inst) return *inst; + + () @trusted { // synchronized {} not @safe for DMD <= 2.078.3 + synchronized { + if (!st_instance.monitor) + st_instance.monitor = new shared NoUseMonitor; + inst = &st_instance; + } + } (); + + return *inst; + } + + override void lock() @safe @nogc nothrow { + assert(false, "Interruptible task mutexes cannot be used with synchronized(), use scopedMutexLock instead."); + } + + override void unlock() @safe @nogc nothrow {} +} + + private void runMutexUnitTests(M)() { import vibe.core.core; From 452fa411c2ae1c5dde49ae4fdece41ebffdf1bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:05:10 +0100 Subject: [PATCH 2/6] Avoid overload conflict when using TaskMutex together with InterruptibleTaskCondition. --- source/vibe/core/sync.d | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/source/vibe/core/sync.d b/source/vibe/core/sync.d index 2710ae0..7ae378d 100644 --- a/source/vibe/core/sync.d +++ b/source/vibe/core/sync.d @@ -699,8 +699,14 @@ final class InterruptibleTaskCondition { private TaskConditionImpl!(true, Lockable) m_impl; - this(core.sync.mutex.Mutex mtx) { m_impl.setup(mtx); } - this(Lockable mtx) { m_impl.setup(mtx); } + this(M)(M mutex) + if (is(M : Mutex) || is (M : Lockable)) + { + static if (is(M : Lockable)) + m_impl.setup(mutex); + else + m_impl.setupForMutex(mutex); + } @property Lockable mutex() { return m_impl.mutex; } void wait() { m_impl.wait(); } @@ -709,6 +715,12 @@ final class InterruptibleTaskCondition { void notifyAll() { m_impl.notifyAll(); } } +unittest { + new InterruptibleTaskCondition(new Mutex); + new InterruptibleTaskCondition(new TaskMutex); + new InterruptibleTaskCondition(new InterruptibleTaskMutex); +} + /** A manually triggered single threaded cross-task event. @@ -1584,7 +1596,7 @@ private struct TaskConditionImpl(bool INTERRUPTIBLE, LOCKABLE) { @trusted bool tryLock() { return m_mutex.tryLock(); } } - void setup(core.sync.mutex.Mutex mtx) + void setupForMutex(core.sync.mutex.Mutex mtx) { setup(new MutexWrapper(mtx)); } From fb64c07d3cfe9690253b1b36ab7521beb059eac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:07:38 +0100 Subject: [PATCH 3/6] Make scopedMutexLock work with InterruptibleTaskMutex. --- source/vibe/core/sync.d | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/source/vibe/core/sync.d b/source/vibe/core/sync.d index 7ae378d..f2fdef5 100644 --- a/source/vibe/core/sync.d +++ b/source/vibe/core/sync.d @@ -33,11 +33,18 @@ shared(ManualEvent) createSharedManualEvent() return shared(ManualEvent).init; } -ScopedMutexLock!M scopedMutexLock(M : Mutex)(M mutex, LockMode mode = LockMode.lock) +ScopedMutexLock!M scopedMutexLock(M)(M mutex, LockMode mode = LockMode.lock) + if (is(M : Mutex) || is(M : Lockable)) { return ScopedMutexLock!M(mutex, mode); } +unittest { + scopedMutexLock(new Mutex); + scopedMutexLock(new TaskMutex); + scopedMutexLock(new InterruptibleTaskMutex); +} + enum LockMode { lock, tryLock, @@ -53,7 +60,8 @@ interface Lockable { /** RAII lock for the Mutex class. */ -struct ScopedMutexLock(M : Mutex = core.sync.mutex.Mutex) +struct ScopedMutexLock(M) + if (is(M : Mutex) || is(M : Lockable)) { @disable this(this); private { From 6c0bdf2976c5d7b876adf3e5271c17c552d9d571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:14:04 +0100 Subject: [PATCH 4/6] Add documentation and unittest example to scopedMutexLock. --- source/vibe/core/sync.d | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/source/vibe/core/sync.d b/source/vibe/core/sync.d index f2fdef5..3b6aec2 100644 --- a/source/vibe/core/sync.d +++ b/source/vibe/core/sync.d @@ -33,12 +33,42 @@ shared(ManualEvent) createSharedManualEvent() return shared(ManualEvent).init; } + +/** Performs RAII based locking/unlocking of a mutex. + + Note that while `TaskMutex` can be used with D's built-in `synchronized` + statement, `InterruptibleTaskMutex` cannot. This function provides a + library based alternative that is suitable for use with all mutex types. +*/ ScopedMutexLock!M scopedMutexLock(M)(M mutex, LockMode mode = LockMode.lock) if (is(M : Mutex) || is(M : Lockable)) { return ScopedMutexLock!M(mutex, mode); } +/// +unittest { + import vibe.core.core : runWorkerTaskH; + + __gshared int counter; + __gshared InterruptibleTaskMutex mutex; + + mutex = new InterruptibleTaskMutex; + + Task[] tasks; + + foreach (i; 0 .. 100) { + tasks ~= runWorkerTaskH({ + auto l = scopedMutexLock(mutex); + counter++; + }); + } + + foreach (t; tasks) t.join(); + + assert(counter == 100); +} + unittest { scopedMutexLock(new Mutex); scopedMutexLock(new TaskMutex); From cccf45cfeacfca4c4f679c0b092171bc54572f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:26:22 +0100 Subject: [PATCH 5/6] Add a motivational introduction to the sync module. Especially mention the issues of using `core.sync.*`. --- source/vibe/core/sync.d | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/source/vibe/core/sync.d b/source/vibe/core/sync.d index 3b6aec2..724a769 100644 --- a/source/vibe/core/sync.d +++ b/source/vibe/core/sync.d @@ -1,7 +1,17 @@ /** - Interruptible Task synchronization facilities + Event loop compatible task synchronization facilities. - Copyright: © 2012-2016 RejectedSoftware e.K. + This module provides replacement primitives for the modules in `core.sync` + that do not block vibe.d's event loop in their wait states. These should + always be preferred over the ones in Druntime under usual circumstances. + + Using a standard `Mutex` is possible as long as it is ensured that no event + loop based functionality (I/O, task interaction or anything that implicitly + calls `vibe.core.core.yield`) is executed within a section of code that is + protected by the mutex. $(B Failure to do so may result in dead-locks and + high-level race-conditions!) + + Copyright: © 2012-2019 Sönke Ludwig Authors: Leonid Kramer, Sönke Ludwig, Manuel Frischknecht License: Subject to the terms of the MIT license, as written in the included LICENSE.txt file. */ @@ -436,11 +446,11 @@ unittest { /** Recursive mutex implementation for tasks. - This mutex type can be used in exchange for a core.sync.mutex.Mutex, but + This mutex type can be used in exchange for a `core.sync.mutex.Mutex`, but does not block the event loop when contention happens. Notice: - Because this class is annotated nothrow, it cannot be interrupted + Because this class is annotated `nothrow`, it cannot be interrupted using $(D vibe.core.task.Task.interrupt()). The corresponding $(D InterruptException) will be deferred until the next blocking operation yields the event loop. @@ -448,7 +458,7 @@ unittest { Use $(D InterruptibleRecursiveTaskMutex) as an alternative that can be interrupted. - See_Also: TaskMutex, core.sync.mutex.Mutex + See_Also: `TaskMutex`, `core.sync.mutex.Mutex` */ final class RecursiveTaskMutex : core.sync.mutex.Mutex, Lockable { @safe: From dfd7d97225d6bc5c319e030436c0cadbf4138878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Mon, 14 Jan 2019 00:27:09 +0100 Subject: [PATCH 6/6] Fix parameter documentation syntax. Fixes #103. --- source/vibe/core/concurrency.d | 4 ++-- source/vibe/core/taskpool.d | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/vibe/core/concurrency.d b/source/vibe/core/concurrency.d index 28ab92e..e0d1326 100644 --- a/source/vibe/core/concurrency.d +++ b/source/vibe/core/concurrency.d @@ -1123,9 +1123,9 @@ struct Future(T) { used and the result is computed within a separate task within the calling thread. Params: - callable: A callable value, can be either a function, a delegate, or a + callable = A callable value, can be either a function, a delegate, or a user defined type that defines an $(D opCall). - args: Arguments to pass to the callable. + args = Arguments to pass to the callable. Returns: Returns a $(D Future) object that can be used to access the result. diff --git a/source/vibe/core/taskpool.d b/source/vibe/core/taskpool.d index 87b0c32..6d85596 100644 --- a/source/vibe/core/taskpool.d +++ b/source/vibe/core/taskpool.d @@ -35,7 +35,7 @@ shared final class TaskPool { /** Creates a new task pool with the specified number of threads. Params: - thread_count: The number of worker threads to create + thread_count = The number of worker threads to create */ this(size_t thread_count = logicalProcessorCount()) @safe {