[monovm] Assertion at object.c:657, condition `lock->done' not met

[jeromelaban]

Description

When running some threaded code in a MAUI app (android in this case), the app may fail with the following error:

[nyname.UnoApp16] * Assertion at /__w/1/s/src/mono/mono/metadata/object.c:657, condition `lock->done' not met

runtime/src/mono/mono/metadata/object.c

Line 657 in cf34ef0

g_assert (lock->done);

This happens when connecting in parallel to multiple websockets, see the StartConnection() in MainPage.xaml.cs.

Reproduction Steps

• Build the following app: 96872-threading-crash.zip
• Start the app with a debugger

Expected behavior

The app does not crash

Actual behavior

the app crashes immediately most of the time

Regression?

Yes, was working fine in 8.0.100.

Known Workarounds

Yes, don't execute any threaded code.

Configuration

dotnet --version
8.0.101

dotnet workload update --print-rollback
==workloadRollbackDefinitionJsonOutputStart==
{
  "microsoft.net.sdk.android": "34.0.52/8.0.100",
  "microsoft.net.sdk.ios": "17.0.8490/8.0.100",
  "microsoft.net.sdk.maccatalyst": "17.0.8490/8.0.100",
  "microsoft.net.sdk.macos": "14.0.8490/8.0.100",
  "microsoft.net.sdk.maui": "8.0.3/8.0.100",
  "microsoft.net.sdk.tvos": "17.0.8490/8.0.100",
  "microsoft.net.workload.mono.toolchain.current": "8.0.1/8.0.100",
  "microsoft.net.workload.emscripten.current": "8.0.1/8.0.100",
  "microsoft.net.workload.emscripten.net6": "8.0.1/8.0.100",
  "microsoft.net.workload.emscripten.net7": "8.0.1/8.0.100",
  "microsoft.net.workload.mono.toolchain.net6": "8.0.1/8.0.100",
  "microsoft.net.workload.mono.toolchain.net7": "8.0.1/8.0.100",
  "microsoft.net.sdk.aspire": "8.0.0-preview.2.23619.3/8.0.100"
}
==workloadRollbackDefinitionJsonOutputEnd==

Other information

No response

[lambdageek]

Probably I messed something up in #93875

[lambdageek]

Oh... yea...

runtime/src/mono/mono/metadata/object.c

Lines 615 to 657 in cf34ef0

	if (!lock->done) {
	int timeout_ms = 500;
	int wait_result = mono_coop_cond_timedwait (&lock->cond, &lock->mutex, timeout_ms);
	if (wait_result == -1) {
	/* timed out - go around again from the beginning. If we got here
	* from the "is_blocked = FALSE" case, above (another thread was
	* blocked on the current thread, but on a lock that was already
	* done but it didn't get to wake up yet), then it might still be
	* the case that the current thread cannot proceed even if the other
	* thread got to wake up - there might be a new deadlock. We need
	* to re-evaluate.
	*
	* This can happen if two threads A and B need to call the cctors
	* for classes X and Y but in opposite orders, and also call a cctor
	* for a third class Z. (That is thread A wants to init: X, Z, Y;
	* thread B wants to init: Y, Z, X.) In that case, if B is waiting
	* for A to finish initializing Z, and A (the current thread )
	* already finished Z and wants to init Y. In A, control will come
	* here with "lock" being Y's lock. But we will time out because B
	* will see that A is responsible for initializing X and will also
	* block. So A is waiting for B to finish Y and B is waiting for A
	* to finish X. So the fact that A allowed B to wait for Z to
	* finish didn't actually let us make progress. Thread A must go
	* around to the top once more and try to init Y - and detect that
	* there is now a deadlock between X and Y.
	*/
	mono_type_init_unlock (lock);
	// clean up blocked thread hash and lock refcount.
	mono_type_initialization_lock ();
	g_hash_table_remove (blocked_thread_hash, GUINT_TO_POINTER (tid));
	gboolean deleted = unref_type_lock (lock);
	if (deleted)
	g_hash_table_remove (type_initialization_hash, vtable);
	mono_type_initialization_unlock ();
	goto retry_top;
	} else if (wait_result == 0) {
	/* Success: we were signaled that the other thread is done. Proceed */
	} else {
	g_assert_not_reached ();
	}
	}
	mono_type_init_unlock (lock);
	g_assert (lock->done);

on line 615 we know !lock->done
then on 650-651 we find out that wait_result == 0 - ie we were signaled on lock->cond. And then we fall through to 656. But we probably need a barrier so that we actually see lock->done (although I thought mutex locking/unlocking is a barrier.)

alternately a thread that is doing the initialization is dying and we're calling mono_release_type_locks which doesn't set lock->done nope... release_type_locks also sets lock->done.

I'll need to try this in a debugger after all. Trying to guess what is wrong isn't working

[lambdageek]

Just noticed another thing that is wrong. when we goto retry_top, we repeat HANDLE_FUNCTION_ENTER() without doing a corresponding HANDLE_FUNCTION_RETURN, so we're incidentally messing up the coop handle stack.

[lambdageek]

Oh. It's probably a spurious wakeup. We need to actually re-evaluate lock->done if wait_result == 0

When using condition variables there is always a Boolean
predicate involving shared variables associated with each
condition wait that is true if the thread should proceed.
Spurious wakeups from the pthread_cond_timedwait() or
pthread_cond_wait() functions may occur. Since the return from
pthread_cond_timedwait() or pthread_cond_wait() does not imply
anything about the value of this predicate, the predicate should
be re-evaluated upon such return.

https://man7.org/linux/man-pages/man3/pthread_cond_wait.3p.html

[lambdageek]

Can repro the crash on a real Pixel 6a, but not in an emulator