Async Method Builder Unexpected Results

[timcassell]

Description

In a console application in Visual Studio 2019, built to .Net Framework 4.7.2 and .Net Core 3.1, given this main method and async method builder, I am getting unexpected results.

AwaitOnCompleted, Task: NULL
AwaitOnCompleted, Task: Type: Promise, State: Pending
Task, Task: Type: Promise, State: Pending
SetResult, Task: NULL

It seems to be setting the task property properly, but then later it's not set. It seems very similar to the IL2CPP bug that Unity has with setting fields on a generic struct, however the method builder in question here doesn't fit that bill.

Interestingly, if I compile and run that exact same code in Unity (except for changing Console.WriteLine to UnityEngine.Debug.Log), it works properly in editor (.Net 4.x scripting runtime), although I have to change it to work with an IL2CPP build (which I didn't expect I would need to do in a VS Console application).

Also, if I change the PromiseMethodBuilder to a class instead of a struct, it works.

AwaitOnCompleted, Task: NULL
AwaitOnCompleted, Task: Type: Promise, State: Pending
Task, Task: Type: Promise, State: Pending
SetResult, Task: Type: Promise, State: Pending

Code

using Proto.Promises;
using System;

namespace PromiseTest
{
    public class Program
    {
        public static void Main(string[] args)
        {
            AsyncPromise();
            Console.ReadKey();
        }

        static void AsyncPromise()
        {
            // Create a promise to await so that the async function won't complete synchronously.
            Promise.Deferred deferred = Promise.NewDeferred();
            Promise promise = deferred.Promise;

            _ = Async();

            async Promise Async()
            {
                await promise;
            }

            deferred.Resolve();
            Promise.Manager.HandleCompletesAndProgress();
        }
    }
}

namespace Proto.Promises.Async.CompilerServices
{
    /// <summary>
    /// This type and its members are intended for use by the compiler.
    /// </summary>
    [DebuggerNonUserCode]
    public struct PromiseMethodBuilder
    {
        private Promise task;

        // Using a promise object as its own continuer saves 16 bytes of object overhead (x64).
        [DebuggerNonUserCode]
        private abstract class AsyncPromise : Promise
        {
            // Cache the delegate to prevent new allocations.
            public readonly Action continuation;

            public AsyncPromise()
            {
                continuation = ContinueMethod;
            }

            protected abstract void ContinueMethod();

            public void SetResult()
            {
                ResolveDirect();
            }

            public void SetException(Exception exception)
            {
                if (exception is OperationCanceledException)
                {
                    CancelDirect(ref exception);
                }
                else
                {
                    RejectDirect(ref exception, int.MinValue);
                }
            }
        }

        [DebuggerNonUserCode]
        private sealed class AsyncPromise<TStateMachine> : AsyncPromise, Internal.ITreeHandleable where TStateMachine : IAsyncStateMachine
        {
            private static ValueLinkedStack<Internal.ITreeHandleable> _pool;

            static AsyncPromise()
            {
                Internal.OnClearPool += () => _pool.Clear();
            }

            private TStateMachine _stateMachine;

            public static AsyncPromise<TStateMachine> GetOrCreate(ref TStateMachine stateMachine)
            {
                var promise = _pool.IsNotEmpty ? (AsyncPromise<TStateMachine>) _pool.Pop() : new AsyncPromise<TStateMachine>();
                promise.Reset();
                promise._stateMachine = stateMachine;
                return promise;
            }

            void Internal.ITreeHandleable.Handle()
            {
                ReleaseInternal();
            }

            protected override void Dispose()
            {
                base.Dispose();
                _stateMachine = default;
                if (Config.ObjectPooling != PoolType.None)
                {
                    _pool.Push(this);
                }
            }

            protected override void ContinueMethod()
            {
                _stateMachine.MoveNext();
            }
        }

        public Promise Task
        {
            get
            {
                Console.WriteLine("Task, Task: " + (task?.ToString() ?? "NULL"));
                return task;
            }
            private set => task = value;
        }

        public static PromiseMethodBuilder Create()
        {
            return new PromiseMethodBuilder();
        }

        public void SetException(Exception exception)
        {
            if (task is null)
            {
                if (exception is OperationCanceledException e)
                {
                    task = Promise.Canceled(e);
                }
                else
                {
                    task = Promise.Rejected(exception);
                }
            }
            else
            {
                ((AsyncPromise) task).SetException(exception);
            }
        }

        public void SetResult()
        {
            Console.WriteLine("SetResult, Task: " + (task?.ToString() ?? "NULL"));
            if (task is null)
            {
                task = Promise.Resolved();
            }
            else
            {
                ((AsyncPromise) task).SetResult();
            }
        }

        public void AwaitOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine)
            where TAwaiter : INotifyCompletion
            where TStateMachine : IAsyncStateMachine
        {
            Console.WriteLine("AwaitOnCompleted, Task: " + (task?.ToString() ?? "NULL"));
            SetContinuation(ref stateMachine);
            Console.WriteLine("AwaitOnCompleted, Task: " + (task?.ToString() ?? "NULL"));
            awaiter.OnCompleted(((AsyncPromise) task).continuation);
        }

        [SecuritySafeCritical]
        public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine)
            where TAwaiter : ICriticalNotifyCompletion
            where TStateMachine : IAsyncStateMachine
        {
            SetContinuation(ref stateMachine);
            awaiter.UnsafeOnCompleted(((AsyncPromise) task).continuation);
        }

        public void Start<TStateMachine>(ref TStateMachine stateMachine)
            where TStateMachine : IAsyncStateMachine
        {
            stateMachine.MoveNext();
        }

        public void SetStateMachine(IAsyncStateMachine stateMachine) { }

        private void SetContinuation<TStateMachine>(ref TStateMachine stateMachine)
            where TStateMachine : IAsyncStateMachine
        {
            if (task is null)
            {
                task = AsyncPromise<TStateMachine>.GetOrCreate(ref stateMachine);
            }
        }
    }
}

Configuration

Visual Studio 2019 16.7.5
.Net Framework 4.7.2 and .Net Core 3.1
Windows 7 SP1, x64
AMD Phenom II x6 @ 2.8 GHz

Regression?

Not sure

Other information

The only workarounds are to either make the method builder a class instead of struct, or set all fields/properties in the Create method, and don't mutate the struct at all in the other methods.

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

[stephentoub]

What compiler version are you using? Does it include #41253?

[timcassell]

What compiler version are you using? Does it include #41253?

I'm not sure, how do I check that? The language version is set to Automatically selected based on framework version.

[EDIT]

C:\Windows\Microsoft.NET\Framework\v4.0.30319>csc
Microsoft (R) Visual C# Compiler version 4.8.3761.0 for C# 5

[stephentoub]

It's in the about box:

That said, I see now in your initial message you said you're using 16.7.5. Assuming you're building from VS / using the VS tools, I believe it should include that change.

Skimming through your code, though, this looks like a bug in your builder. The compiler is going to generate code something like this as the entrypoint to the async method:

<M>d__0 stateMachine = default(<M>d__0);
stateMachine.<>t__builder = AsyncTaskMethodBuilder.Create();
stateMachine.<>1__state = -1;
stateMachine.<>t__builder.Start(ref stateMachine);
return stateMachine.<>t__builder.Task;

so the Task that's accessed and returned to the caller is off of the builder that's inside of the state machine that is on the stack and that was passed by ref to Start. If I'm following the logic in your pasted code correctly, though, you never update that ref stateMachine with the task you create; rather, you end up only mutating a copy of it.

[stephentoub]

If I'm following the logic in your pasted code correctly, though, you never update that ref stateMachine with the task you create; rather, you end up only mutating a copy of it.

For reference, you can see that AsyncTaskMethodBuilder passes a ref to its m_task:
https://github.com/dotnet/runtime/blob/912e88d02e7d9beab0f165233e881e7880310912/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs#L88
and in addition to updating the state machine field, first stores the created task into that ref:
https://github.com/dotnet/runtime/blob/912e88d02e7d9beab0f165233e881e7880310912/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs#L226-L227
so that the caller sees the created task as well.

[timcassell]

Hm, I do see the potential logic flaw with not updating the ref stateMachine in AwaitOnCompleted, but I have seen it working
without updating that cached stateMachine (and I actually followed someone else's example of doing that to cache the stateMachine). So it seems I should actually cache it in Start to begin with, then update it in AwaitOnCompleted, and that makes sense.

However, what doesn't make sense is that the task field is not set when SetResult is called, as if the builder is copied to another location during the stateMachine.MoveNext() call, thus causing the AwaitOnCompleted and Task to be called on a different instance than SetResult, which I don't really see how that makes sense either, if the code you pasted is what is generated (I would expect the Task call to also have an unset task field).

I'll give that change a try and see if it works, but I still think something fishy is going on here with the builder instance for each method.

[EDIT] Also, I purposefully did not cache the stateMachine in Start in case the async method itself completed synchronously (hence the check to see if the task is null in SetResult)

[timcassell]

Actually, I'm a little confused what I'm doing wrong here. It looks like I'm doing the same thing Tasks are with setting the task in AwaitOnCompleted, the only difference is they are setting the task field on the builder first, whereas I'm setting the stateMachine on the promise object before I set the task field to that object, but I'm not sure what difference that makes...

Hm, interesting, so I changed my SetContinuation to this, and now it's working:

private void SetContinuation<TStateMachine>(ref TStateMachine stateMachine)
    where TStateMachine : IAsyncStateMachine
{
    if (task is null)
    {
        var promise = AsyncPromise<TStateMachine>.GetOrCreate();
        task = promise;
        promise.stateMachine = stateMachine;
    }
}

If I swap the task = and promise.stateMachine = it doesn't work. I see there was a comment in the Task builder there that that order is "very important", but logically I don't understand why.

[EDIT] Ok, now I think I understand why. Setting the task field is actually modifying the stateMachine itself, so if I set the promise.stateMachine first, it's making a copy of the statemachine struct before I mutate the this. But... why did the old code work in Unity?

[stephentoub]

But... why did the old code work in Unity?

Don't know. Maybe there were some differences, e.g. things written and/or compiled as classes instead of structs.