AOP proxies: release targets and pop the exposed proxy when the call throws

src/Spring/Spring.Aop/Aop/Framework/AutoProxy/AbstractAutoProxyCreator.cs

@@ -427,8 +427,6 @@ protected virtual object CreateProxy(Type targetType, string targetName, IList<o
         // copy our properties (proxyTargetClass) inherited from ProxyConfig
         proxyFactory.CopyFrom(this);
 
-        object target = targetSource.GetTarget();
-
         if (!ProxyTargetType)
         {
             // Must allow for introductions; can't just set interfaces to

src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs

@@ -101,6 +101,12 @@ public abstract class AbstractAopProxyMethodBuilder : AbstractProxyMethodBuilder
 
     private static Dictionary<Type, OpCode> ldindOpCodes;
 
+    /// <summary>
+    /// Records whether this call pushed the proxy on the <see cref="AopContext"/> stack
+    /// (i.e. whether <c>ExposeProxy</c> was set), so that exactly the calls that pushed also pop.
+    /// </summary>
+    private LocalBuilder proxyExposed;
+
     static AbstractAopProxyMethodBuilder()
     {
         ldindOpCodes = new Dictionary<Type, OpCode>();
@@ -172,6 +178,11 @@ protected AbstractAopProxyMethodBuilder(
     /// <summary>
     /// Generates the proxy method.
     /// </summary>
+    /// <remarks>
+    /// The method logic (<see cref="GenerateMethodLogic"/>) is emitted inside a try region whose
+    /// finally pops the exposed proxy from the <see cref="AopContext"/> stack. An override that
+    /// bypasses this method must therefore pop the proxy itself, on every path out of the call.
+    /// </remarks>
     /// <param name="il">The IL generator to use.</param>
     /// <param name="method">The method to proxy.</param>
     /// <param name="interfaceMethod">
@@ -188,7 +199,17 @@ protected override void GenerateMethod(
         GenerateOnProxyTargetMethodCacheField(il, method);
 
         BeginMethod(il, method);
+
+        // Everything between the push and the pop of the exposed proxy runs inside a try,
+        // and the pop is in the finally, so the AopContext stack is restored even when the
+        // advice chain or the target throws. The stack is empty at both ends of the region
+        // and the result lives in the returnValue local, which EndMethod loads afterwards.
+        il.BeginExceptionBlock();
         GenerateMethodLogic(il, method, interfaceMethod);
+        il.BeginFinallyBlock();
+        PopExposedProxy(il);
+        il.EndExceptionBlock();
+
         EndMethod(il, method);
     }
 
@@ -346,6 +367,7 @@ protected virtual void DeclareLocals(ILGenerator il, MethodInfo method)
         interceptors = il.DeclareLocal(typeof(System.Collections.IList));
         targetType = il.DeclareLocal(typeof(Type));
         arguments = il.DeclareLocal(typeof(Object[]));
+        proxyExposed = il.DeclareLocal(typeof(bool));
 
         if (method.IsGenericMethodDefinition)
         {
@@ -383,17 +405,11 @@ protected virtual void DeclareLocals(ILGenerator il, MethodInfo method)
     /// <param name="method">The method to proxy.</param>
     protected virtual void BeginMethod(ILGenerator il, MethodInfo method)
     {
-        Label jmpProxyNotExposed = il.DefineLabel();
-
-        // set current proxy to this object
+        // remember whether the proxy is exposed, so the pop mirrors the push
         PushAdvisedProxy(il);
         il.Emit(OpCodes.Ldfld, References.AdvisedField);
         il.EmitCall(OpCodes.Callvirt, References.ExposeProxyProperty, null);
-        il.Emit(OpCodes.Brfalse_S, jmpProxyNotExposed);
-        il.Emit(OpCodes.Ldarg_0);
-        il.EmitCall(OpCodes.Call, References.PushProxyMethod, null);
-
-        il.MarkLabel(jmpProxyNotExposed);
+        il.Emit(OpCodes.Stloc, proxyExposed);
 
         // initialize targetType
         PushTargetType(il);
@@ -405,11 +421,43 @@ protected virtual void BeginMethod(ILGenerator il, MethodInfo method)
         PushTargetMethodInfo(il, method);
         il.EmitCall(OpCodes.Call, References.GetInterceptorsMethod, null);
         il.Emit(OpCodes.Stloc, interceptors);
+
+        // set current proxy to this object; this comes last, right before the protected
+        // region, so nothing that can fail sits between the push and the try
+        Label jmpProxyNotExposed = il.DefineLabel();
+        il.Emit(OpCodes.Ldloc, proxyExposed);
+        il.Emit(OpCodes.Brfalse_S, jmpProxyNotExposed);
+        il.Emit(OpCodes.Ldarg_0);
+        il.EmitCall(OpCodes.Call, References.PushProxyMethod, null);
+
+        il.MarkLabel(jmpProxyNotExposed);
+    }
+
+    /// <summary>
+    /// Emits the code that resets the current proxy to its old value, if this call exposed it.
+    /// Runs in the finally block of the method.
+    /// </summary>
+    /// <param name="il">The IL generator to use.</param>
+    private void PopExposedProxy(ILGenerator il)
+    {
+        Label jmpProxyNotExposed = il.DefineLabel();
+
+        il.Emit(OpCodes.Ldloc, proxyExposed);
+        il.Emit(OpCodes.Brfalse_S, jmpProxyNotExposed);
+        il.EmitCall(OpCodes.Call, References.PopProxyMethod, null);
+
+        il.MarkLabel(jmpProxyNotExposed);
     }
 
     /// <summary>
     /// Generates method logic.
     /// </summary>
+    /// <remarks>
+    /// The emitted code runs inside try/finally regions (see <see cref="GenerateMethod"/>), so it must
+    /// leave the evaluation stack empty, keep the result in the return value local, and must not
+    /// emit <c>ret</c> or branch out of the region. Overrides that acquire a resource (such as the
+    /// target) should release it in a finally block of their own.
+    /// </remarks>
     /// <param name="il">The IL generator to use.</param>
     /// <param name="method">The method to proxy.</param>
     /// <param name="interfaceMethod">
@@ -564,23 +612,17 @@ protected abstract void CallDirectProxiedMethod(
         ILGenerator il, MethodInfo method, MethodInfo interfaceMethod);
 
     /// <summary>
-    /// Ends method by returning return value if appropriate.
+    /// Ends method by loading the return value, if appropriate.
     /// </summary>
+    /// <remarks>
+    /// This runs after the try/finally regions, which have already popped the exposed proxy from
+    /// the <see cref="AopContext"/> stack. It must not emit <c>ret</c>; the caller does.
+    /// </remarks>
     /// <param name="il">The IL generator to use.</param>
     /// <param name="method">The method to proxy.</param>
     protected virtual void EndMethod(ILGenerator il, MethodInfo method)
     {
-        Label jmpProxyNotExposed = il.DefineLabel();
-
-        // reset current proxy to old value
-        PushAdvisedProxy(il);
-        il.Emit(OpCodes.Ldfld, References.AdvisedField);
-        il.EmitCall(OpCodes.Callvirt, References.ExposeProxyProperty, null);
-        il.Emit(OpCodes.Brfalse_S, jmpProxyNotExposed);
-        il.EmitCall(OpCodes.Call, References.PopProxyMethod, null);
-
-        il.MarkLabel(jmpProxyNotExposed);
-
+        // the exposed proxy has already been reset in the finally block (see GenerateMethod)
         if (methodReturnsValue)
         {
             il.Emit(OpCodes.Ldloc, returnValue);

src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyTypeBuilder.cs

@@ -32,6 +32,13 @@ public abstract class AbstractAopProxyTypeBuilder :
     /// Generates the IL instructions that pushes
     /// the target instance on which calls should be delegated to.
     /// </summary>
+    /// <remarks>
+    /// This default borrows a target with <see cref="ITargetSource.GetTarget"/> and emits no matching
+    /// <see cref="ITargetSource.ReleaseTarget"/>. No builder in this library reaches it: the
+    /// composition and decorator method builders keep the target in a local and release it themselves,
+    /// and the inheritance builder overrides it. A derived builder that relies on this default with a
+    /// pooled or otherwise non-static target source must release the target itself.
+    /// </remarks>
     /// <param name="il">The IL generator to use.</param>
     public override void PushTarget(ILGenerator il)
     {

src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/BaseCompositionAopProxy.cs

@@ -82,38 +82,28 @@ object IAopProxy.GetProxy()
     /// <returns>true if the specified Object is equal to the current target object; otherwise, false</returns>
     public override bool Equals(object obj)
     {
-        bool equals = false;
         object target = m_targetSource.GetTarget();
-        AdvisedProxy otherProxy = obj as AdvisedProxy;
-        object otherTarget = null;
-        if (otherProxy != null)
+        try
         {
-            otherTarget = otherProxy.m_targetSource.GetTarget();
-            if (target == null)
+            if (obj is AdvisedProxy otherProxy)
             {
-                equals = (otherTarget == null);
+                object otherTarget = otherProxy.m_targetSource.GetTarget();
+                try
+                {
+                    return target == null ? otherTarget == null : target.Equals(otherTarget);
+                }
+                finally
+                {
+                    otherProxy.m_targetSource.ReleaseTarget(otherTarget);
+                }
             }
-            else
-            {
-                equals = target.Equals(otherTarget);
-            }
-        }
-        else if (target == null)
-        {
-            equals = (obj == null);
-        }
-        else
-        {
-            equals = target.Equals(obj);
-        }
 
-        m_targetSource.ReleaseTarget(target);
-        if (otherProxy != null)
+            return target == null ? obj == null : target.Equals(obj);
+        }
+        finally
         {
-            otherProxy.m_targetSource.ReleaseTarget(otherTarget);
+            m_targetSource.ReleaseTarget(target);
         }
-
-        return equals;
     }
 
     /// <summary>
@@ -122,15 +112,15 @@ public override bool Equals(object obj)
     /// <returns>A hash code for the target object.</returns>
     public override int GetHashCode()
     {
-        int hashCode = 0;
         object target = m_targetSource.GetTarget();
-        if (target != null)
+        try
         {
-            hashCode = target.GetHashCode();
+            return target != null ? target.GetHashCode() : 0;
+        }
+        finally
+        {
+            m_targetSource.ReleaseTarget(target);
         }
-
-        m_targetSource.ReleaseTarget(target);
-        return hashCode;
     }
 
     /// <summary>
@@ -139,18 +129,14 @@ public override int GetHashCode()
     /// <returns>A String that represents the target object</returns>
     public override string ToString()
     {
-        string str;
         object target = m_targetSource.GetTarget();
-        if (target != null)
+        try
         {
-            str = target.ToString();
+            return target != null ? target.ToString() : base.ToString();
         }
-        else
+        finally
         {
-            str = base.ToString();
+            m_targetSource.ReleaseTarget(target);
         }
-
-        m_targetSource.ReleaseTarget(target);
-        return str;
     }
 }

src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/TargetAopProxyMethodBuilder.cs

@@ -94,12 +94,23 @@ protected override void GenerateMethodLogic(
         il.EmitCall(OpCodes.Callvirt, References.GetTargetMethod, null);
         il.Emit(OpCodes.Stloc, target);
 
+        // The target has been borrowed (it may come from a pool), so it must go back
+        // on every path out of the invocation, including when the advice chain or the
+        // target method throws. The evaluation stack is empty at both ends of the
+        // protected region, results live in locals (returnValue, arguments), and
+        // BeginExceptionBlock/EndExceptionBlock emit the required 'leave' instructions.
+        il.BeginExceptionBlock();
+
         base.GenerateMethodLogic(il, method, interfaceMethod);
 
+        il.BeginFinallyBlock();
+
         PushAdvisedProxy(il);
         il.Emit(OpCodes.Ldfld, References.TargetSourceField);
         PushTarget(il);
         il.EmitCall(OpCodes.Callvirt, References.GetReleaseTargetMethod, null);
+
+        il.EndExceptionBlock();
     }
 
     /// <summary>

test/Spring/Spring.Aop.Tests/Aop/Framework/AutoProxy/AutoProxyTargetSourceBorrowTests.cs

@@ -0,0 +1,83 @@
+/*
+ * Copyright 2002-2010 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using NUnit.Framework;
+using Spring.Aop.Framework.DynamicProxy;
+using Spring.Aop.Interceptor;
+using Spring.Objects.Factory;
+using Spring.Objects.Factory.Support;
+
+namespace Spring.Aop.Framework.AutoProxy;
+
+/// <summary>
+/// Creating an auto-proxy must not borrow a target from a custom <see cref="ITargetSource"/>
+/// (or, if it ever does, must give it back).
+/// </summary>
+[TestFixture]
+public class AutoProxyTargetSourceBorrowTests
+{
+    public class Service
+    {
+        public virtual int Answer() => 42;
+    }
+
+    private sealed class CountingTargetSourceCreator : ITargetSourceCreator
+    {
+        public readonly MockTargetSource TargetSource = new MockTargetSource();
+
+        public ITargetSource GetTargetSource(Type objectType, string objectName, IObjectFactory factory)
+        {
+            TargetSource.SetTarget(new Service());
+            return TargetSource;
+        }
+    }
+
+    private sealed class NopAutoProxyCreator : AbstractAutoProxyCreator
+    {
+        public NopAutoProxyCreator(IObjectFactory objectFactory)
+        {
+            ObjectFactory = objectFactory;
+        }
+
+        protected override IList<object> GetAdvicesAndAdvisorsForObject(Type targetType, string targetName, ITargetSource customTargetSource)
+        {
+            return new List<object> { new NopInterceptor() };
+        }
+    }
+
+    [Test]
+    public void CreatingProxyDoesNotBorrowFromTargetSource()
+    {
+        DefaultListableObjectFactory objectFactory = new DefaultListableObjectFactory();
+        objectFactory.RegisterObjectDefinition("service", new RootObjectDefinition(typeof(Service)));
+
+        CountingTargetSourceCreator creator = new CountingTargetSourceCreator();
+        NopAutoProxyCreator autoProxyCreator = new NopAutoProxyCreator(objectFactory);
+        autoProxyCreator.CustomTargetSourceCreators = new List<object> { creator };
+        objectFactory.AddObjectPostProcessor(autoProxyCreator);
+
+        Service proxy = (Service) objectFactory.GetObject("service");
+
+        Assert.That(AopUtils.IsAopProxy(proxy), Is.True);
+        Assert.That(creator.TargetSource.gets, Is.EqualTo(0), "creating the proxy borrowed a target");
+        Assert.That(creator.TargetSource.releases, Is.EqualTo(creator.TargetSource.gets));
+
+        // and the proxy still borrows and releases per call
+        Assert.That(proxy.Answer(), Is.EqualTo(42));
+        Assert.That(creator.TargetSource.gets, Is.EqualTo(1));
+        Assert.That(creator.TargetSource.releases, Is.EqualTo(creator.TargetSource.gets));
+    }
+}