For a couple of weeks now I have been using the Async CTP and I really like it. But this post is not about the things I liked but a subtle race condition I found.

So I have created this application that takes a resource from the pool does some async communication then puts the resource back to the pool in a finally clause. Every test case passed and all was great. But when I wrote a test case that put a lot more load on the system I started to get strange corruptions. After some investigation I found that the problem was that sometimes my finally clause was executed before my main code was finished resulting in corrupted state when two threads was using the same resource at the same time. Below is my repro case that produces the error without doing much else.

class Program
{
    static async Task<int> AsyncMethod1()
    {
        int counter = 2;

        int one = await AsyncMethod2();
        await TaskEx.Yield();

        try
        {
            int two = await AsyncMethod3();
            await TaskEx.Yield();

            if (Interlocked.Decrement(ref counter) == 0)
            {
                Console.WriteLine("Error");
            }

            return one + two;
        }
        finally
        {
            Interlocked.Decrement(ref counter);
        }
    }

    static async Task<int> AsyncMethod2()
    {
        await TaskEx.Yield();
        return 1;
    }

    static async Task<int> AsyncMethod3()
    {
        await TaskEx.Yield();
        return 2;
    }

    static void Main(string[] args)
    {
        const int Count = 10000000;
        var tasks = new Task<int>[Count];

        for (int i = 0; i < Count; i++)
        {
            tasks[i] = AsyncMethod1();
        }

        Task.WaitAll(tasks);

        Console.WriteLine("DONE");
        Console.ReadKey();
    }
}

As you can see you should not be able to get the application to write “Error” to the console, but this not the case. So let’s dig a little deeper and look at why this is happening. Below is the code for the method AsyncMethod1 produced by the compiler:

private sealed class <AsyncMethod1>d__0
{
    private bool $__disposing;
    private bool $__doFinallyBodies;
    // ...
    public void MoveNext()
    {
        try
        {
            this.$__doFinallyBodies = true;
            switch (this.<>1__state)
            {
                // ...
                default:
                    this.<counter>5__1 = 2;
                    this.<a1>t__$await5 = Program.AsyncMethod2().GetAwaiter<int>();
                    this.<>1__state = 1;
                    this.$__doFinallyBodies = false;
                    if (this.<a1>t__$await5.BeginAwait(this.MoveNextDelegate))
                    {
                        return;
                    }
                    this.$__doFinallyBodies = true;
                    break;
            }
            this.<>1__state = 0;
            this.<1>t__$await4 = this.<a1>t__$await5.EndAwait();
            this.<one>5__2 = this.<1>t__$await4;
            this.<a2>t__$await6 = TaskEx.Yield().GetAwaiter();
            this.<>1__state = 2;
            this.$__doFinallyBodies = false;
            if (this.<a2>t__$await6.BeginAwait(this.MoveNextDelegate))
            {
                return;
            }
            this.$__doFinallyBodies = true;
        Label_00DC:
            this.<>1__state = 0;
            this.<a2>t__$await6.EndAwait();
        Label_00EE:
            try
            {
                switch (this.<>1__state)
                {
                    case 3:
                        break;

                    case 4:
                        goto Label_01A7;

                    default:
                        this.<a3>t__$await8 = Program.AsyncMethod3().GetAwaiter<int>();
                        this.<>1__state = 3;
                        this.$__doFinallyBodies = false;
                        if (this.<a3>t__$await8.BeginAwait(this.MoveNextDelegate))
                        {
                            return;
                        }
                        this.$__doFinallyBodies = true;
                        break;
                }
                this.<>1__state = 0;
                this.<3>t__$await7 = this.<a3>t__$await8.EndAwait();
                this.<two>5__3 = this.<3>t__$await7;
                this.<a4>t__$await9 = TaskEx.Yield().GetAwaiter();
                this.<>1__state = 4;
                this.$__doFinallyBodies = false;
                if (this.<a4>t__$await9.BeginAwait(this.MoveNextDelegate))
                {
                    return;
                }
                this.$__doFinallyBodies = true;
            Label_01A7:
                this.<>1__state = 0;
                this.<a4>t__$await9.EndAwait();
                if (Interlocked.Decrement(ref this.<counter>5__1) == 0)
                {
                    Console.WriteLine("Error");
                }
                this.<>1__state = -1;
                this.$builder.SetResult(this.<one>5__2 + this.<two>5__3);
                return;
            }
            finally
            {
                if (this.$__doFinallyBodies)
                {
                    Interlocked.Decrement(ref this.<counter>5__1);
                }
            }
        }
        catch (Exception exception)
        {
            this.<>1__state = -1;
            this.$builder.SetException(exception);
        }
    }
}

As you can see our finally clause is only executed if the field this $__doFinallyBodies is true but the finally clause of the generated code is executed for each state in the state machine:

finally
{
    if (this.$__doFinallyBodies)
    {
        Interlocked.Decrement(ref this.<counter>5__1);
    }
}

So have you found the subtle race condition?

The problem is that $__doFinallyBodies is a field shared by each state in the state machine instead of a method variable, so when two thread at the same time is executing the MoveNext() method the first setting $__doFinallyBodis to true and the second is entering the finally clause expecting $__doFinallyBodis to be false. When this happens our finally clause will be executed before is should and in my test app “Error” is written to the console and in my original app the resource is now used by two threads at the same time and all sort of cool things is happening.

I think this example shows how easy it is to miss shared state in an application resulting in a fairly hard to find bug.