Mono: `Monitor.Enter()` is not interrupted by `Thread.Interrupt()`

[kouvel]

See MonitorTests.InterruptWaitTest under src/libraries/System.Threading/tests (the test is being added by PR #87672):

runtime/src/libraries/System.Threading/tests/MonitorTests.cs

Lines 493 to 511 in ed64748

	public static void InterruptWaitTest()
	{
	object obj = new();
	lock (obj)
	{
	var threadReady = new AutoResetEvent(false);
	var t =
	ThreadTestHelpers.CreateGuardedThread(out Action waitForThread, () =>
	{
	threadReady.Set();
	Assert.Throws<ThreadInterruptedException>(() => Monitor.Enter(obj));
	});
	t.IsBackground = true;
	t.Start();
	threadReady.CheckedWait();
	t.Interrupt();
	waitForThread();
	}
	}

[lambdageek]

Not a regression. Same behavior with mono on net6.0 and net7.0.

Works on legacy Mono 6.12.0.188.

It looks like we still have the code for async thread interruptions and the WaitSleepJoin thread state. But we don't have this block anymore in dotnet/runtime mono

https://github.com/mono/mono/blob/59a40d68879b69416f15a1d78198483cddda8cc0/mono/metadata/threads.c#L5161-L5170

[lambdageek]

That code was removed as part of #47333

I'm not sure if there was some corert-based interruption logic that was wired up that didn't work, or if it worked initially and then we added something before the net6 release that broke it

[lambdageek]

@kouvel This is probably going to be hard to fix. The right thing is to finish out #48058 and remove mono's native Monitor implementation in favor of a managed one like NativeAOT.

But touching anything related to the sync block causes significant performance perturbations. I think we can probably switch to using a Lock object in Mono instead of the native Monitor, but I'm less sure about switching from an inline sync block to a conditional weak table.

The runtime code for interruptable syscalls is still there, so that part of the native monitor implementation is still functional. It's just that we don't do anything with that information and simply retry the syscall as if we got a spurious interruption. I suppose one thing we can do is call out to managed to WaitSubsystem.ThreadWaitInfo.CheckAndResetPendingInterrupt_NotLocked (same as a Sleep0) and check whether there is a pending interrupt. I'm not sure if that kind of hack is worth it. (Also I'm not 100% confident that it won't mess up the WaitSubsystem invariants)

[lambdageek]

Made a draft PR that piggybacks on the interruption logic in Mono and calls out to thread.WaitInfo.CheckAndResetPendingInterrupt_TakeLock (which takes s_Lock and then checks the wait subsystem interruption flag)

it's not too bad - it won't get called unless the condvar wait syscall is actually interrupted for some reason (most commonly, I think, due to a GC) and calling out to managed is not unreasonable at that point.

I will play around with actually replacing native Monitor logic with a managed Lock, too, but I think that's probably too big of a change for net8 at this point