When Method passes parameters by reference Then Advice Around does not work correctly

[dilandau2001]

Environment (please complete the following information):

• OS: Windows
• Framework: tested with net462, and NET8.0, same issue
• Type of application: Library
• Version of AspectInjector: 2.8.2

Describe the bug
When using a Advice(Kind.Around) to meassure time consumed by methods.
When method sets properties passed by reference. Probably it might happen also with out references.
When the ref property is set, the change does not occur until the whole advice finishes.

To Reproduce
Create a class like this:

//[LogCall]
public class MyClass
{
    /// <summary>
    /// Random integer property for the test.
    /// </summary>
    private int _property;

    /// <summary>
    /// Gets or sets the Property.
    /// </summary>
    public int Property
    {
        get { return _property; }
        set { SetProperty(ref _property, value); }
    }

    /// <summary>
    /// Event that is fired when property changes.
    /// </summary>
    public event EventHandler PropertyChanged;

    /// <summary>
    /// Help function to set the property.
    /// </summary>
    /// <param name="property"></param>
    /// <param name="value"></param>
    private void SetProperty(ref int property, int value)
    {
        if (property == value)
            return;

        property = value;
        // Note we are firing the event AFTER updating the reference.
        PropertyChanged?.Invoke(this, EventArgs.Empty);
    }
}

Create the aspect like this:

[Aspect(Scope.PerInstance)]
[Injection(typeof(LogCall))]
public class LogCall : Attribute
{
    [Advice(Kind.Around, Targets = Target.Method)]
    public object MeasureExecutionTime(
        [Argument(Source.Target)] Func<object[], object> methodInvocation,
        [Argument(Source.Name)] string methodName,
        [Argument(Source.Arguments)] object[] args)
    {
        var stopwatch = new Stopwatch();
        stopwatch.Start();

        // Capture the method result or just invoke if void
        object returnValue;
        try
        {
            returnValue = methodInvocation(args);
        }
        finally
        {
            stopwatch.Stop();
            Console.WriteLine($"{methodName} took {stopwatch.ElapsedMilliseconds} ms");
        }

        return returnValue;
    }
}

And create a unit test like this:

[TestClass]
public class TestingClass
{
    [TestMethod]
    public void WhenCaptureThenUpdated()
    {
        // Arrange
        var testedClass = new MyClass();
        int result = 0;
        testedClass.PropertyChanged += (sender, args) => result = testedClass.Property;

        // Act
        testedClass.Property = 5;

        // Assert
        Assert.AreEqual(5, testedClass.Property);
        Assert.AreEqual(5, result);
    }
}

This unit tests works fine if LogCall is commented out, and it fails otherwise in this assertion: Assert.AreEqual(5, result);
When paremeters are passed by reference or out, calling methodInvocation(args) is not really updating at the moment you would expect. It is like if a copy of the value is the one being passed but it does update when the advice finishes because this passes: Assert.AreEqual(5, testedClass.Property);

Additional context
What I am trying to create is an aspect that is able to meassure how long it takes for each method to complete its work, for performance analysis.
So far I have tried 2 approachs.
First I tried to use Advice.Before and Advice.After, but I found no way to share the stopwatcher in a thread safe way. Also this approach does not work when there is an exception within the method. We lack a [Argument(Source.Context)] that could be used as a dictionary to store objects, so we can store an instance in the Before, and use it in the after. Also After should fire when theere is an exception in the method.
Then I tried the advice.Around, which look promising but was failing in runtime with many nullreference exceptions. I guess the aspect should be able to support a type of Source of type MethodDelegate, so you can tell it to just proceed, instead of invoking it though the Func<object[], object>(object[] arguments)

[pamidur]

Hey @dilandau2001, I have reviewed your report and dug dipper into it and I must admit this a design flaw in a way ref/out params are deconstructed and given as object[] to an Around method. The expectation (wrong) was that the Around injection can replace ref for the time of execution and then redirect the result to a proper refs once the method is finished without the original caller ever notice that.

Something like orig call passing ref -> around infrastructure substitutes the ref -> aspect user code can manipulate object[] args -> original method -> around infrastructure substitutes the ref on the way back -> original caller has its ref properly working

The problem is (now) obviously that changes to a field passed by ref won't be recorded until after the original method and whole aspect chain is completed. Which wasn't a problem for mere variables passed by ref.

As I said, this is a design flaw, there is no fix (as far as I can tell, It'd require a complete rewrite and changes to API that deals with arguments), I'll need to record this issue in the docs.

Thanks and I'm sorry

[dilandau2001]

Fine. I see using the "Around" method won't be possible.
But, in order to achieve my real objective, it would be enough to have some type of "context" that I could use to share the stopwatch instance between the begin aspect and the end aspect. Do you think this other approach could be possible?

[pamidur]

It is technically possible, yes, but requires some major redesign. I might have a look at this at a later time

…

On Tue, 10 Dec 2024, 09:41 dilandau2001, ***@***.***> wrote: Fine. I see using the "Around" method won't be possible. But, in order to achieve my real objective, it would be enough to have some type of "context" that I could use to share the stopwatch instance between the begin aspect and the end aspect. Do you think this other approach could be possible? — Reply to this email directly, view it on GitHub <#235 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7HZUHSIJCMTKTFDGIELYD2EYMDHAVCNFSM6AAAAABO4GNBLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRZGY4DSNZZGE> . You are receiving this because you commented.Message ID: ***@***.***>