InterruptibleTaskMutex with InterruptibleTaskCondition

[WebFreak001]

I'm not quite sure why this code wouldn't work? It just throws an unhelpful assert error:

core.exception.AssertError@../../.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/sync.d(1453): Assertion failure
----------------
??:? _d_assertp [0xc40c38bc]
../../.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/sync.d:1453 nothrow @trusted void vibe.core.sync.TaskMutexImpl!(true).TaskMutexImpl.unlock() [0xc40572c1]
../../.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/sync.d:383 nothrow @safe void vibe.core.sync.InterruptibleTaskMutex.unlock() [0xc40513bc]
../../.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/sync.d:1565 @trusted void vibe.core.sync.TaskConditionImpl!(true, vibe.core.sync.Lockable).TaskConditionImpl.wait() [0xc4057dc6]
../../.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/sync.d:663 @safe void vibe.core.sync.InterruptibleTaskCondition.wait() [0xc40516d4]
source/app.d:28 @trusted immutable(char)[] app.calculate(immutable(char)[]) [0xc403966c]
source/app.d:14 pure @nogc @safe immutable(char)[] app.main().__dgliteral2() [0xc40395c7]
/home/webfreak/.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/log.d:131 pure @nogc @safe immutable(char)[] vibe.core.log.logInfo!("source/app.d", 14, immutable(char)[], immutable(char)[]).logInfo(immutable(char)[], lazy immutable(char)[]).__dgliteral4() [0xc40450c4]
/home/webfreak/.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/log.d:807 nothrow @safe void vibe.core.log.doLog!(immutable(char)[], immutable(char)[]).doLog(vibe.core.log.LogLevel, immutable(char)[], immutable(char)[], immutable(char)[], int, immutable(char)[], lazy immutable(char)[]) [0xc4045100]
/home/webfreak/.dub/packages/vibe-core-1.4.6/vibe-core/source/vibe/core/log.d:131 nothrow @safe void vibe.core.log.logInfo!("source/app.d", 14, immutable(char)[], immutable(char)[]).logInfo(immutable(char)[], lazy immutable(char)[]) [0xc404509a]
source/app.d:14 _Dmain [0xc4039577]
There were still 2 tasks running at exit.
Program exited with code 1

Any reason for this design decision? Maybe an error message should be added or it should immediately catch and abort TaskMutex inside TaskCondition, this has caused multiple issues to me already and it seems just using a normal Mutex works fine without issues even though it's all one thread?

import vibe.core.core;
import vibe.core.log;
import vibe.core.sync;
import core.time;

void main()
{
	runTask({
		while (true) {
			logInfo("k");
			sleep(1.seconds);
		}
	});
	logInfo("Result: %s", calculate());
}

string calculate()
{
	import core.sync.mutex : Mutex;

	auto mutex = new InterruptibleTaskMutex(); // <-- everything works if you do new Mutex instead
	auto condition = new InterruptibleTaskCondition(mutex);

	runTask({
		sleep(10.seconds);
		condition.notify();
	});

	synchronized (mutex)
		condition.wait();

	return "done";
}

[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(3v/u) INF] k
[main(----) INF] Result: done

[s-ludwig]

The issue is a bit subtle here, as the code indeed looks totally fine from the outside. The problem comes from the fact that InterruptibleTaskMutex cannot inherit from Mutex, because the lock method is not nothrow. Consequently, synchronized (mutex) will not call mutex.lock(), but mutex._monitor.lock(), which is not the mutex that the condition tries to unlock afterwards.

I'd love to get rid of the built-in monitor object in this case to at least get an error message in the synchronized line, but I'm not sure whether that is possible. But the fix here is to use { auto l = scopedMutexLock(); condition.wait() } instead.

[s-ludwig]

Alternatively it would probably also be valid to use a normal TaskMutex, unless any I/O/yield is going on while the mutex is locked.

[WebFreak001]

what would happen if I use a normal mutex? In that code everything seems to work fine and even the background task on the same thread is still running while the mutex is locked

[s-ludwig]

As long as nothing blocks for an extended period of time during the lock, that would be fine, too. In that case it is an imperative to not yield in any form within the lock, as that would be a deadlock-trap or (high-level) race-condition depending on whether the mutex is recursive. You can add auto l = yieldLock(); in the same scope to verify that at runtime.

TaskMutex would still work correctly in the presence of calls to yield, the task would then just not be interruptible while the lock is contended.

[WebFreak001]

uh could you post some more full example code? This all seems a bit too error prone to deadlocks to me if I just try to apply your comments. I don't know what a scopedMutexLock is or how to use it with my mutex or condition.

[s-ludwig]

The TaskMutex version would look exactly like the example above, just replacing the InterruptibleTaskMutex. This is what I'd basically always use myself.

InterruptibleTaskMutex + scopedLock looks like this:

string calculate()
{
	import core.sync.mutex : Mutex;

	auto mutex = new InterruptibleTaskMutex();
	auto condition = new InterruptibleTaskCondition(mutex);

	runTask({
		sleep(10.seconds);
		condition.notify();
	});

	{
		auto l = scopedLock(mutex);
		// ... can do something else here ...
		condition.wait();
		// ... can do something else here ...
	}

	return "done";
}

Mutex + yieldLock would look like this (definitely the most error prone variant, but may be viable if all that is done within the lock is checking a simple condition or setting a plain variable, in which case of course the yieldLock is also unnecessary):

string calculate()
{
	import core.sync.mutex : Mutex;

	auto mutex = new Mutex();
	auto condition = new InterruptibleTaskCondition(mutex);

	runTask({
		sleep(10.seconds);
		condition.notify();
	});

	synchronized (mutex) {
		{
			auto l = yieldLock();
			// ... can do something else here ...
		}
		condition.wait();
		{
			auto l = yieldLock();
			// ... can do something else here ...
		}
	}

	return "done";
}

[WebFreak001]

ah ok. Can a TaskMutex be used to lock in multiple threads?

I think these questions I had here should be included in the documentation, it's rather sparse. Could the TaskCondition constructor maybe check what is being passed and raise a compile time issue for InterrutibleTaskMutex?

[s-ludwig]

Yes, TaskMutex is supposed to be thread-safe. Good point about the documentation, usage examples and mentioning the gotchas is an important omission.

Regarding InterruptibleTaskMutex, the problem is that using it for TaskCondition is totally fine. The problematic spot is just using it with synchronized - maybe there is some way to detect/prevent this, but I don't really see how, since accessing Object.monitor happens completely outside of control of the InterruptibleTaskMutex class. Adding an invariant that checks this.monitor is null would be possible, but it's not clear whether the class invariant will be called at all and it wouldn't give a hint about where in the program the monitor was created.

[WebFreak001]

Doesn't the TaskMutex become interruptible when being used with InterruptibleTaskCondition because the condition unlocks the mutex and can be interrupted in that time?

Let's rename this issue to just adding some more documentation to the vibe.core.sync parts then.

One more thing about usage with TaskMutex: there are overloads taking Lockable and Mutex which both match, I just cast to Lockable now and it works, is there any difference in using Mutex?

[s-ludwig]

Hm, I just got an idea. InterruptibleTaskMutex could just set a dummy monitor instance in its constructor. This dummy instance would then just assert out on use.

[s-ludwig]

Doesn't the TaskMutex become interruptible when being used with InterruptibleTaskCondition because the condition unlocks the mutex and can be interrupted in that time?

That would be the TaskCondition that is interruptible then, but the mutex would still not be interruptible during the call to lock(), if for example some lengthy I/O is going on within the lock.

One more thing about usage with TaskMutex: there are overloads taking Lockable and Mutex which both match, I just cast to Lockable now and it works, is there any difference in using Mutex?

Strange I never noticed this, I'll look into it. Both overloads should work the same, though.

[s-ludwig]

PR: #119