pamidur / aspect-injector

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

Allow Aspects.Universal to work with generic return types #178

Closed wssmith closed 2 years ago

wssmith commented 2 years ago

Environment (please complete the following information):

Describe the bug An InvalidCastException is thrown when a method with a MethodAspect attribute and generic return type is called multiple times with different return type arguments.

To Reproduce

public class LogCall : MethodAspectAttribute
{
    protected override void OnBefore(AspectEventArgs eventArgs)
        => Console.WriteLine($"OnBefore method {eventArgs.Name}");

    protected override void OnAfter(AspectEventArgs eventArgs)
        => Console.WriteLine($"OnAfter method {eventArgs.Name}");

    protected override T OnException<T>(AspectEventArgs eventArgs, Exception exception)
    {
        Console.WriteLine($"OnException method {eventArgs.Name} --> {exception}");
        throw exception;
    }
}

public class Program
{
    private static void Main()
    {
        try
        {   
            int n = Create<int>(); // OK
            bool b = Create<bool>(); // ERROR: invalid cast from bool to int
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }
    }

    [LogCall]
    private static T Create<T>() where T : new() => new();
}

Additional context The method handler cache in BaseUniversalWrapperAspect is using MethodBase objects as keys. Since the equality of these keys isn't affected by type arguments, every instantiation of a generic method ends up using the same handler. This leads to problems if any instantiations have different return types. Adding the return type to the cache's key ensures a different handler is built for each return type.

pamidur commented 2 years ago

Hi @wssmith , nice improvement, why closed?

wssmith commented 2 years ago

Hi @wssmith , nice improvement, why closed?

When I realized there was a thread-safety issue too, I closed the PR planning to reopen it after I replaced the Dictionary and locking with ConcurrentDictionary and Lazy. With "lock (method)" it was still possible for threads to add entries to the Dictionary unsynchronized.

I'm satisfied with the changes now. Let me know if you have questions or concerns.

wssmith commented 2 years ago

Could you please as well update all Aspects to use latest AspectInjector in Directory.Build.props ?

I've updated it now.