Async iterators permit yield return inside of lock blocks

[stephentoub]

Version Used:
d8d7886

Steps to Reproduce:
SharpLab

await new C().ProcessValueAsync();

public class C
{
    public async Task ProcessValueAsync()
    {
        await foreach (int item in GetValuesAsync())
        {
            await Task.Delay(1);
        }
    }

    private async IAsyncEnumerable<int> GetValuesAsync()
    {
        await Task.Yield();
        lock (this)
        {
            for (int i = 0; i < 10; i++)
            {
                yield return i;
            }
        }
    }
}

Expected Behavior:
Fails to compile, with an error about not yielding while holding a lock.

Actual Behavior:
Compiles without error, but blows up at runtime with an exception:

Unhandled exception. System.Threading.SynchronizationLockException: Object synchronization method was called from an unsynchronized block of code.
   at System.Threading.Monitor.Exit(Object obj)
   at C.GetValuesAsync()+MoveNext()

Since normal synchronous IEnumerable<T> iterators were introduced, yield return has unfortunately been allowed while holding a lock. This is problematic because it means the caller iterating through the iterator will also be holding the lock, e.g. this:

object o = new object();
Console.WriteLine($"Before: {Monitor.IsEntered(o)}");
using (IEnumerator<int> e = GetValues(o).GetEnumerator())
{
    Console.WriteLine($"Inside: {Monitor.IsEntered(o)}");
    while (e.MoveNext())
    {
        Console.WriteLine($"{e.Current}: {Monitor.IsEntered(o)}");
    }
    Console.WriteLine($"Done: {Monitor.IsEntered(o)}");
}
Console.WriteLine($"After: {Monitor.IsEntered(o)}");

static IEnumerable<int> GetValues(object obj)
{
    lock (obj)
    {
        for (int i = 0; i < 3; i++)
            yield return i;
    }
}

prints this:

Before: False
Inside: False
0: True
1: True
2: True
Done: False
After: False

It appears async iterators have inherited the same lack of constraint around using yield return inside of a lock, but the problems are compounded because with an async iterator, the caller will very likely be await'ing other things while iterating, which then means that it's likely the eventual call into the async iterator's MoveNext that releases the lock will be on a different thread than the one which acquired it. Moreover, in the interim other work that runs on the thread that was holding the lock will see that it's holding the lock, and will be able to observe any invariants that might not no longer hold. Async methods and async iterators disallow using await inside of a lock, but allowing yield return can effectively amount to the same thing, since the caller will be await'ing things.

Ideally this would have been a hard error. If it's too late to do that, we should really have a warning about yield return inside of a lock, ideally for both async and sync iterators, but at a minimum for async.

[jaredpar]

@jjonescz let's address this as part of relaxing the restrictions around ref struct and unsafe in iterators / async methods. Given how long it's been in the compiler I think moving to a hard error is going to be hard to do. Initially at least should use a warning. Can consider upping the severity in .NET 10 to error.

[jjonescz]

This has been discussed in LDM, Jun 3, 2024, as part of review of the ref-in-async feature.

The conclusion was that we should remove this warning to avoid a breaking change (especially given our recent breaking changes, we don't want to make the situation worse) and it was pointed out that this could be an analyzer instead.

The decision wasn't final because it was pointed out that yield return is disallowed in a try block (and lock generates a try block). But it turns out that's only for try/catch, it's normally allowed in try/finally, and it's also allowed in using/foreach. So I think that point is not relevant and I should proceed with removing the warning.

cc @jaredpar @AlekseyTs @cston

[stephentoub]

The conclusion was that we should remove this warning to avoid a breaking change (especially given our recent breaking changes, we don't want to make the situation worse) and it was pointed out that this could be an analyzer instead.

I'm sorry I had to leave the meeting early. 😦

it was pointed out that this could be an analyzer instead

Is there a fundamental difference between it being implemented in the compiler and it being implemented in an analyzer that ships in the .NET SDK and is enabled by default?