Capgemini / Cauldron

C# Toolkit
MIT License
76 stars 18 forks source link

Async Method Weaving: OnEnter() is called multiple times #66

Closed Dhairya-Sangoi closed 5 years ago

Dhairya-Sangoi commented 5 years ago

Hey,

It seems there is a bug in weaving async methods.

Attribute:

[InterceptorOptions(AlwaysCreateNewInstance = true)]
    public class TestingAnnotation : Attribute, IMethodInterceptor
    {
        public void OnEnter(Type declaringType, object instance, MethodBase methodbase, object[] values)
        {

        }

        public bool OnException(Exception e)
        {
            return true;
        }

        public void OnExit()
        {

        }
    }

Annotated class method:

    class TestingClass
    {
        [TestingAnnotation]
        public async Task AsyncTestMethod(string inp1, int inp2)
        {
            Console.WriteLine("inside async test method");
            await Task.Delay(100);
            Console.WriteLine("inside async test method");
            await Task.Delay(100);
            Console.WriteLine("inside async test method");
            await Task.Delay(100);
        }
    }

Decompiled weaved code:

[CompilerGenerated]
    private sealed class \u003CAsyncTestMethod\u003Ed__0 : IAsyncStateMachine
    {
      public int \u003C\u003E1__state;
      public AsyncTaskMethodBuilder \u003C\u003Et__builder;
      public string inp1;
      public int inp2;
      public TestingClass \u003C\u003E4__this;
      private TaskAwaiter \u003C\u003Eu__1;
      public IMethodInterceptor \u003CAsyncTestMethod\u003E_2yaukd2v55stk;

      public \u003CAsyncTestMethod\u003Ed__0()
      {
        base.\u002Ector();
      }

      void IAsyncStateMachine.MoveNext()
      {
        int num1 = this.\u003C\u003E1__state;
        try
        {
          try
          {
            object[] values = new object[2]
            {
              (object) this.inp1,
              (object) this.inp2
            };
            if (num1 != 0)
            {
              // ISSUE: method reference
              // ISSUE: type reference
              this.\u003CAsyncTestMethod\u003E_2yaukd2v55stk.OnEnter(typeof (TestingClass), (object) this.\u003C\u003E4__this, MethodBase.GetMethodFromHandle(__methodref (TestingClass.AsyncTestMethod), __typeref (TestingClass)), values);
            }
            TaskAwaiter awaiter1;
            int num2;
            TaskAwaiter awaiter2;
            TaskAwaiter awaiter3;
            switch (num1)
            {
              case 0:
                awaiter1 = this.\u003C\u003Eu__1;
                this.\u003C\u003Eu__1 = new TaskAwaiter();
                this.\u003C\u003E1__state = num1 = -1;
                break;
              case 1:
                awaiter2 = this.\u003C\u003Eu__1;
                this.\u003C\u003Eu__1 = new TaskAwaiter();
                this.\u003C\u003E1__state = num1 = -1;
                goto label_10;
              case 2:
                awaiter3 = this.\u003C\u003Eu__1;
                this.\u003C\u003Eu__1 = new TaskAwaiter();
                this.\u003C\u003E1__state = num1 = -1;
                goto label_13;
              default:
                Console.WriteLine("inside async test method");
                awaiter1 = Task.Delay(100).GetAwaiter();
                if (!awaiter1.IsCompleted)
                {
                  this.\u003C\u003E1__state = num2 = 0;
                  this.\u003C\u003Eu__1 = awaiter1;
                  TestingClass.\u003CAsyncTestMethod\u003Ed__0 stateMachine = this;
                  this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<TaskAwaiter, TestingClass.\u003CAsyncTestMethod\u003Ed__0>(ref awaiter1, ref stateMachine);
                  return;
                }
                break;
            }
            awaiter1.GetResult();
            Console.WriteLine("inside async test method");
            awaiter2 = Task.Delay(100).GetAwaiter();
            if (!awaiter2.IsCompleted)
            {
              this.\u003C\u003E1__state = num2 = 1;
              this.\u003C\u003Eu__1 = awaiter2;
              TestingClass.\u003CAsyncTestMethod\u003Ed__0 stateMachine = this;
              this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<TaskAwaiter, TestingClass.\u003CAsyncTestMethod\u003Ed__0>(ref awaiter2, ref stateMachine);
              return;
            }
label_10:
            awaiter2.GetResult();
            Console.WriteLine("inside async test method");
            awaiter3 = Task.Delay(100).GetAwaiter();
            if (!awaiter3.IsCompleted)
            {
              this.\u003C\u003E1__state = num2 = 2;
              this.\u003C\u003Eu__1 = awaiter3;
              TestingClass.\u003CAsyncTestMethod\u003Ed__0 stateMachine = this;
              this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<TaskAwaiter, TestingClass.\u003CAsyncTestMethod\u003Ed__0>(ref awaiter3, ref stateMachine);
              return;
            }
label_13:
            awaiter3.GetResult();
            if (num1 != 0)
              ;
          }
          finally
          {
            this.\u003CAsyncTestMethod\u003E_2yaukd2v55stk.OnExit();
          }
        }
        catch (Exception ex)
        {
          if (this.\u003CAsyncTestMethod\u003E_2yaukd2v55stk.OnException(ex))
          {
            this.\u003C\u003E1__state = -2;
            this.\u003C\u003Et__builder.SetException(ex);
            return;
          }
        }
        this.\u003C\u003E1__state = -2;
        this.\u003C\u003Et__builder.SetResult();
      }

      [DebuggerHidden]
      void IAsyncStateMachine.SetStateMachine(IAsyncStateMachine stateMachine)
      {
      }
    }

As it can be seen, the call to OnEnter is based on the following condition:

if (num1 != 0)
            {
              // ISSUE: method reference
              // ISSUE: type reference
              this.\u003CAsyncTestMethod\u003E_2yaukd2v55stk.OnEnter(typeof (TestingClass), (object) this.\u003C\u003E4__this, MethodBase.GetMethodFromHandle(__methodref (TestingClass.AsyncTestMethod), __typeref (TestingClass)), values);
            }

I think, the condition should be this instead:

if (num1 == -1)
            {
              // ISSUE: method reference
              // ISSUE: type reference
              this.\u003CAsyncTestMethod\u003E_2yaukd2v55stk.OnEnter(typeof (TestingClass), (object) this.\u003C\u003E4__this, MethodBase.GetMethodFromHandle(__methodref (TestingClass.AsyncTestMethod), __typeref (TestingClass)), values);
            }

num1 is the current state of the state machine which is created for this method. A call to OnEnter should only happen if the current state is the initial state (-1).

reflection-emit commented 5 years ago

I uploaded a new package with the fix. Please confirm

Dhairya-Sangoi commented 5 years ago

Thanks for the quick fix. Will confirm and let you know.

Dhairya-Sangoi commented 5 years ago

Confirmed. Works as expected! Thanks for the quick fix :)