pamidur / aspect-injector

AOP framework for .NET (c#, vb, etc)
Apache License 2.0
742 stars 111 forks source link

VB After/Surround Instance async aspect fails at runtime, "Object reference not set to an instance of an object." #182

Closed Meigs2 closed 2 years ago

Meigs2 commented 2 years ago

Environment (please complete the following information):

Describe the bug Async generic methods with aspects targeting after or around and scope set to "Instance" in async method fails at runtime with a "Object reference not set to an instance of an object."

To Reproduce See samples below

Additional context While I was investigating my issues with #173 and #174, which may be resolved, I noticed I was able to get vb projects to compile, but some vb tests were failing with a "Object reference not set to an instance of an object." error seemingly on the invoke to the after or around generated method. Global instances did not have this issue, but Instances did.

After lots of debugging and isolation, it seems the problem most likely lies with the differences in VB state machine generation compared to C# and generics, specifically in MoveNext.

VB appears to generate slightly different code compared to C#, adding an extra local variable, and performing some slightly different operations, which I believe is causing the error, however I am not knowledgeable enough with IL and Cecil to diagnose the exact issue.


(Full source files will be linked at the end)

This is the method and call that is failing at runtime at End Function (Pardon the possibly confusing or poor naming, this is pulled from my local branch):

Dim result = Await vb.TestAsyncMethod("Test")
...
<InjectInstanceAspect>
Public Async Function TestAsyncMethod(Of T)(ByVal obj As T) As Task(Of T)
    Return Await Task.FromResult(obj)
End Function

This function and call does not fail at runtime:

Dim result = Await vb.TestAsyncMethod("Test")
...
<InjectInstanceAspect>
Public Async Function TestAsyncMethod(ByVal obj As String) As Task(Of String)
    Return Await Task.FromResult(obj)
End Function

The C# equivalents of these calls do not fail, see at the bottom.

Using Rider's IL window, I pulled the differences in IL between each project, with and without the aspect injected. The original in the diff is WTHOUT aspects injected, and the midified is WITH aspects injected.

Again, excuse the possible poor naming.

Visual Basic with non-generic string param (PASSES):

IL diff: https://www.diffchecker.com/Hyo27tr3

Code:

Imports AspectInjector.Broker
Imports AspectInjector.Tests.Assets
Imports Xunit

Public Class _9999
    <Fact>
    Public Async Sub AdviceBefore_Methods_Passes()
        Dim vb = New TestClassVbAspect()
        Dim result = Await vb.TestAsyncMethod("Test")
        Assert.Equal("Test", result)
    End Sub

    Partial Friend NotInheritable Class TestClassVbAspect
        <InjectInstanceAspect>
        Public Async Function TestAsyncMethod(Of String)(ByVal obj As String) As Task(Of String)
            Return Await Task.FromResult(obj)
        End Function
    End Class

    <AttributeUsage(AttributeTargets.All, AllowMultiple:=True)>
    <Injection(GetType(InstanceAspect))>
    Public Class InjectInstanceAspect
        Inherits Attribute
    End Class

    <Aspect(Scope.PerInstance)>
    Public Class InstanceAspect
        Inherits TestAspectBase

'        <Advice(Kind.After)>
'        Public Sub AspectMethod()
'        End Sub

    End Class
End Class

Visual Basic generic param (FAILS WITH EXCEPTION AT END FUNCTION AFTER INJECTION):

IL diff: https://www.diffchecker.com/dandGSgM

Code:

Imports AspectInjector.Broker
Imports AspectInjector.Tests.Assets
Imports Xunit

Public Class _9999
    <Fact>
    Public Async Sub AdviceBefore_Methods_Passes()
        Dim vb = New TestClassVbAspect()
        Dim result = Await vb.TestAsyncMethod("Test")
        Assert.Equal("Test", result)
    End Sub

    Partial Friend NotInheritable Class TestClassVbAspect
        <InjectInstanceAspect>
        Public Async Function TestAsyncMethod(Of T)(ByVal obj As T) As Task(Of T)
            Return Await Task.FromResult(obj)
        End Function
    End Class

    <AttributeUsage(AttributeTargets.All, AllowMultiple:=True)>
    <Injection(GetType(InstanceAspect))>
    Public Class InjectInstanceAspect
        Inherits Attribute
    End Class

    <Aspect(Scope.PerInstance)>
    Public Class InstanceAspect
        Inherits TestAspectBase

'        <Advice(Kind.After)>
'        Public Sub AspectMethod()
'        End Sub

    End Class
End Class

C# with non-generic string param (PASSES):

IL diff: https://www.diffchecker.com/ZGEZQAas

Code:

using System;
using System.Threading.Tasks;
using AspectInjector.Broker;
using AspectInjector.Tests.Assets;
using Xunit;

public class _9999
{
    [Fact]
    public async void AdviceBefore_Methods_Passes()
    {
        var vb = new TestClassVbAspect();
        var result = await vb.TestAsyncMethod("Test");
        Assert.Equal("Test", result);
    }

    internal sealed partial class TestClassVbAspect
    {
        [InjectInstanceAspect]
        public async Task<string> TestAsyncMethod(string obj)
        {
            return await Task.FromResult(obj);
        }
    }

    [AttributeUsage(AttributeTargets.All, AllowMultiple = true)]
    [Injection(typeof(InstanceAspect))]
    public class InjectInstanceAspect : Attribute
    {
    }

    [Aspect(Scope.PerInstance)]
    public class InstanceAspect : TestAspectBase
    {
        [Advice(Kind.After)]
        public void AspectMethod()
        {
        }
    }
}

C# generic param (PASSES):

IL diff: https://www.diffchecker.com/2ZDgwsSf

Code:

using System;
using System.Threading.Tasks;
using AspectInjector.Broker;
using AspectInjector.Tests.Assets;
using Xunit;

public class _9999
{
    [Fact]
    public async void AdviceBefore_Methods_Passes()
    {
        var vb = new TestClassVbAspect();
        var result = await vb.TestAsyncMethod("Test");
        Assert.Equal("Test", result);
    }

    internal sealed partial class TestClassVbAspect
    {
        [InjectInstanceAspect]
        public async Task<T> TestAsyncMethod<T>(T obj)
        {
            return await Task.FromResult(obj);
        }
    }

    [AttributeUsage(AttributeTargets.All, AllowMultiple = true)]
    [Injection(typeof(InstanceAspect))]
    public class InjectInstanceAspect : Attribute
    {
    }

    [Aspect(Scope.PerInstance)]
    public class InstanceAspect : TestAspectBase
    {
        // [Advice(Kind.After)]
        // public void AspectMethod()
        // {
        // }
    }
}

Possibly important things to note

You can see if the diffs that the VB MoveNext state machine methods generate an extra parameter of the return type and uses it elsewhere, while C# only generates a single param of the return type in MoveNext. The nullref is probably thrown when we try and load that into the after method call, but I am unsure why. Boxing issue?

pamidur commented 2 years ago

HI @Meigs2 , This is great report - investigation. Let me take a look at this issue closely

Meigs2 commented 2 years ago

Whoops, first sample still used generics in vb. Updated. The IL diff is still accurate.

pamidur commented 2 years ago

@Meigs2 could you please try version 2.7.3-pre1 and if it fixes anything?

Meigs2 commented 2 years ago

I also forgot to mention the changes required to the async state machine identification required but it seems you found it in 7e58016.

Meigs2 commented 2 years ago

@Meigs2 could you please try version 2.7.3-pre1 and if it fixes anything?

Will do.

Meigs2 commented 2 years ago

Looks like this issue in particular is corrected as of 2.7.3-pre1, my tests for this issue pass. However some of my other runtime tests for VB are failing. Should I close this issue and open another?

Link to branch: https://github.com/Meigs2/aspect-injector/tree/182

pamidur commented 2 years ago

Yes, let's make one ticket per issue. This one fixes "After Async method for VB" scenario

pamidur commented 2 years ago

fixed in 2.7.3-pre1