From ba36617bfe899d1d40aff377555b00fb66a8da41 Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Wed, 4 Nov 2026 00:37:43 +0200 Subject: [PATCH 1/3] Packet A1: release pooled/borrowed AOP targets when the advised call throws TargetAopProxyMethodBuilder emitted GetTarget ... ReleaseTarget without a try/finally, so a throwing advised method leaked pooled targets (the pool eventually ran dry and blocked). The invocation is now wrapped in an IL try/finally. BaseCompositionAopProxy.Equals/GetHashCode/ToString get the same treatment in C#. AbstractAutoProxyCreator.CreateProxy borrowed a target it never used or released; the borrow is removed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LGkLQuAuUjvRjrU1AcTsmk Signed-off-by: Marko Lahma --- .../AutoProxy/AbstractAutoProxyCreator.cs | 2 - .../DynamicProxy/BaseCompositionAopProxy.cs | 64 ++-- .../TargetAopProxyMethodBuilder.cs | 11 + .../AutoProxyTargetSourceBorrowTests.cs | 83 +++++ .../DynamicProxy/ReleaseTargetTests.cs | 287 ++++++++++++++++++ 5 files changed, 406 insertions(+), 41 deletions(-) create mode 100644 test/Spring/Spring.Aop.Tests/Aop/Framework/AutoProxy/AutoProxyTargetSourceBorrowTests.cs create mode 100644 test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs diff --git a/src/Spring/Spring.Aop/Aop/Framework/AutoProxy/AbstractAutoProxyCreator.cs b/src/Spring/Spring.Aop/Aop/Framework/AutoProxy/AbstractAutoProxyCreator.cs index 4f6b29113..0f1c807a9 100644 --- a/src/Spring/Spring.Aop/Aop/Framework/AutoProxy/AbstractAutoProxyCreator.cs +++ b/src/Spring/Spring.Aop/Aop/Framework/AutoProxy/AbstractAutoProxyCreator.cs @@ -427,8 +427,6 @@ protected virtual object CreateProxy(Type targetType, string targetName, IListtrue if the specified Object is equal to the current target object; otherwise, false 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; } /// @@ -122,15 +112,15 @@ public override bool Equals(object obj) /// A hash code for the target object. 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; } /// @@ -139,18 +129,14 @@ public override int GetHashCode() /// A String that represents the target object 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; } } diff --git a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/TargetAopProxyMethodBuilder.cs b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/TargetAopProxyMethodBuilder.cs index e551bc3d2..15aec79ac 100644 --- a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/TargetAopProxyMethodBuilder.cs +++ b/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(); } /// diff --git a/test/Spring/Spring.Aop.Tests/Aop/Framework/AutoProxy/AutoProxyTargetSourceBorrowTests.cs b/test/Spring/Spring.Aop.Tests/Aop/Framework/AutoProxy/AutoProxyTargetSourceBorrowTests.cs new file mode 100644 index 000000000..db0f4c71a --- /dev/null +++ b/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; + +/// +/// Creating an auto-proxy must not borrow a target from a custom +/// (or, if it ever does, must give it back). +/// +[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 GetAdvicesAndAdvisorsForObject(Type targetType, string targetName, ITargetSource customTargetSource) + { + return new List { 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 { 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)); + } +} diff --git a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs new file mode 100644 index 000000000..0b00cd7fe --- /dev/null +++ b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs @@ -0,0 +1,287 @@ +/* + * 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.Interceptor; +using Spring.Aop.Target; +using Spring.Objects.Factory.Support; + +namespace Spring.Aop.Framework.DynamicProxy; + +/// +/// Proxies borrow their target from the for the duration of a call, +/// so they have to give it back on every path out of the call, including when it throws. +/// +[TestFixture] +public class ReleaseTargetTests +{ + public struct ProbeStruct + { + public int Value; + } + + public interface IReleaseProbe + { + void DoVoid(); + string GetString(); + int GetInt(); + ProbeStruct GetStruct(); + void RefOut(ref int counter, out string text); + void Throw(); + int ThrowInt(); + ProbeStruct ThrowStruct(); + string ThrowString(); + } + + public class ReleaseProbe : IReleaseProbe + { + public int VoidCalls; + + public virtual void DoVoid() => VoidCalls++; + + public virtual string GetString() => "text"; + + public virtual int GetInt() => 42; + + public virtual ProbeStruct GetStruct() => new ProbeStruct { Value = 7 }; + + public virtual void RefOut(ref int counter, out string text) + { + counter += 10; + text = "out:" + counter; + } + + public virtual void Throw() => throw new InvalidOperationException("boom"); + + public virtual int ThrowInt() => throw new InvalidOperationException("boom"); + + public virtual ProbeStruct ThrowStruct() => throw new InvalidOperationException("boom"); + + public virtual string ThrowString() => throw new InvalidOperationException("boom"); + + public bool ThrowFromObjectMethods; + + public override bool Equals(object obj) => ThrowFromObjectMethods ? throw new InvalidOperationException("boom") : base.Equals(obj); + + public override int GetHashCode() => ThrowFromObjectMethods ? throw new InvalidOperationException("boom") : base.GetHashCode(); + + public override string ToString() => ThrowFromObjectMethods ? throw new InvalidOperationException("boom") : base.ToString(); + } + + private sealed class ThrowingInterceptor : AopAlliance.Intercept.IMethodInterceptor + { + public object Invoke(AopAlliance.Intercept.IMethodInvocation invocation) + { + throw new InvalidOperationException("advice boom"); + } + } + + private static IReleaseProbe CreateProxy(ITargetSource targetSource, bool decorator, bool advised, AopAlliance.Aop.IAdvice advice = null) + { + ProxyFactory factory = new ProxyFactory(); + factory.TargetSource = targetSource; + if (decorator) + { + factory.ProxyTargetType = true; + } + else + { + factory.AddInterface(typeof(IReleaseProbe)); + } + + if (advice != null) + { + factory.AddAdvice(advice); + } + else if (advised) + { + factory.AddAdvice(new NopInterceptor()); + } + + object proxy = factory.GetProxy(); + Assert.That(AopUtils.IsDecoratorAopProxy(proxy), Is.EqualTo(decorator)); + return (IReleaseProbe) proxy; + } + + private static MockTargetSource CreateCountingTargetSource(out ReleaseProbe probe) + { + probe = new ReleaseProbe(); + MockTargetSource targetSource = new MockTargetSource(); + targetSource.SetTarget(probe); + return targetSource; + } + + private static void AssertBalanced(MockTargetSource targetSource, int expectedCalls) + { + Assert.That(targetSource.gets, Is.EqualTo(expectedCalls)); + Assert.That(targetSource.releases, Is.EqualTo(targetSource.gets)); + } + + // decorator, advised + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ReleasesTargetAfterVoidCall(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out ReleaseProbe probe); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised); + + proxy.DoVoid(); + + Assert.That(probe.VoidCalls, Is.EqualTo(1)); + AssertBalanced(targetSource, 1); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ReleasesTargetAfterReferenceTypeReturn(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised); + + Assert.That(proxy.GetString(), Is.EqualTo("text")); + + AssertBalanced(targetSource, 1); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ReleasesTargetAfterValueTypeReturn(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised); + + Assert.That(proxy.GetInt(), Is.EqualTo(42)); + Assert.That(proxy.GetStruct().Value, Is.EqualTo(7)); + + AssertBalanced(targetSource, 2); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ReleasesTargetAfterRefAndOutParameters(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised); + + int counter = 5; + proxy.RefOut(ref counter, out string text); + + Assert.That(counter, Is.EqualTo(15)); + Assert.That(text, Is.EqualTo("out:15")); + AssertBalanced(targetSource, 1); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ReleasesTargetWhenTargetMethodThrows(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised); + + Assert.That(() => proxy.Throw(), Throws.TypeOf().With.Message.EqualTo("boom")); + AssertBalanced(targetSource, 1); + + Assert.That(() => proxy.ThrowInt(), Throws.TypeOf()); + AssertBalanced(targetSource, 2); + + Assert.That(() => proxy.ThrowStruct(), Throws.TypeOf()); + AssertBalanced(targetSource, 3); + + Assert.That(() => proxy.ThrowString(), Throws.TypeOf()); + AssertBalanced(targetSource, 4); + } + + [Test] + public void CompositionProxyReleasesTargetWhenObjectMethodsThrow() + { + MockTargetSource targetSource = CreateCountingTargetSource(out ReleaseProbe probe); + IReleaseProbe proxy = CreateProxy(targetSource, decorator: false, advised: true); + MockTargetSource otherTargetSource = CreateCountingTargetSource(out _); + IReleaseProbe otherProxy = CreateProxy(otherTargetSource, decorator: false, advised: true); + probe.ThrowFromObjectMethods = true; + + Assert.That(() => proxy.Equals(new object()), Throws.TypeOf()); + AssertBalanced(targetSource, 1); + + Assert.That(() => proxy.Equals(otherProxy), Throws.TypeOf()); + AssertBalanced(targetSource, 2); + AssertBalanced(otherTargetSource, 1); + + Assert.That(() => proxy.GetHashCode(), Throws.TypeOf()); + AssertBalanced(targetSource, 3); + + Assert.That(() => proxy.ToString(), Throws.TypeOf()); + AssertBalanced(targetSource, 4); + } + + [TestCase(false)] + [TestCase(true)] + public void ReleasesTargetWhenAdviceThrows(bool decorator) + { + MockTargetSource targetSource = CreateCountingTargetSource(out ReleaseProbe probe); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised: true, new ThrowingInterceptor()); + + Assert.That(() => proxy.DoVoid(), Throws.TypeOf().With.Message.EqualTo("advice boom")); + Assert.That(() => proxy.GetInt(), Throws.TypeOf()); + + Assert.That(probe.VoidCalls, Is.EqualTo(0)); + AssertBalanced(targetSource, 2); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void PoolIsNotExhaustedByCallsThatThrow(bool decorator, bool advised) + { + DefaultListableObjectFactory objectFactory = new DefaultListableObjectFactory(); + objectFactory.RegisterObjectDefinition("probe", new RootObjectDefinition(typeof(ReleaseProbe), false)); + + SimplePoolTargetSource pool = new SimplePoolTargetSource(); + pool.TargetObjectName = "probe"; + pool.MaxSize = 1; + pool.ObjectFactory = objectFactory; + try + { + IReleaseProbe proxy = CreateProxy(pool, decorator, advised); + + // an exhausted pool of one blocks the next borrower, so every call + // has to hand its target back before the next one is made + for (int i = 0; i < 2; i++) + { + Assert.That(() => proxy.Throw(), Throws.TypeOf()); + Assert.That(pool.Active, Is.EqualTo(0), "target was not released after call " + i); + } + + proxy.DoVoid(); + Assert.That(pool.Active, Is.EqualTo(0)); + } + finally + { + pool.Dispose(); + } + } +} From 14fb12e379d59df8e0935dd09ad41f3ad8588964 Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Wed, 4 Nov 2026 00:41:03 +0200 Subject: [PATCH 2/3] Packet A1: pop the exposed AopContext proxy in a finally AbstractAopProxyMethodBuilder pushed the proxy on the AopContext stack when ExposeProxy was set and popped it at the end of the method, outside any finally, so every call that threw left a stale entry on the thread's stack. The method body now runs in an IL try whose finally pops the proxy. The push moves to the end of BeginMethod (after the local initialisation, before the try), and ExposeProxy is read once into a local so the pop mirrors the push. The target release (inner finally) runs before the pop (outer finally). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LGkLQuAuUjvRjrU1AcTsmk Signed-off-by: Marko Lahma --- .../AbstractAopProxyMethodBuilder.cs | 65 +++++++---- .../DynamicProxy/ReleaseTargetTests.cs | 103 +++++++++++++++++- 2 files changed, 148 insertions(+), 20 deletions(-) diff --git a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs index fefdc77a6..007f631c7 100644 --- a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs +++ b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs @@ -101,6 +101,12 @@ public abstract class AbstractAopProxyMethodBuilder : AbstractProxyMethodBuilder private static Dictionary ldindOpCodes; + /// + /// Records whether this call pushed the proxy on the stack + /// (i.e. whether ExposeProxy was set), so that exactly the calls that pushed also pop. + /// + private LocalBuilder proxyExposed; + static AbstractAopProxyMethodBuilder() { ldindOpCodes = new Dictionary(); @@ -188,7 +194,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 +362,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 +400,11 @@ protected virtual void DeclareLocals(ILGenerator il, MethodInfo method) /// The method to proxy. 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,6 +416,32 @@ 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); + } + + /// + /// 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. + /// + /// The IL generator to use. + 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); } /// @@ -570,17 +607,7 @@ protected abstract void CallDirectProxiedMethod( /// The method to proxy. 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); diff --git a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs index 0b00cd7fe..724afcc00 100644 --- a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs +++ b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs @@ -44,6 +44,17 @@ public interface IReleaseProbe int ThrowInt(); ProbeStruct ThrowStruct(); string ThrowString(); + void CallInnerThenCaptureProxy(); + } + + public interface IThrowingIntroduction + { + void ThrowIntroduced(); + } + + public class ThrowingIntroduction : IThrowingIntroduction, AopAlliance.Aop.IAdvice + { + public void ThrowIntroduced() => throw new InvalidOperationException("boom"); } public class ReleaseProbe : IReleaseProbe @@ -72,6 +83,22 @@ public virtual void RefOut(ref int counter, out string text) public virtual string ThrowString() => throw new InvalidOperationException("boom"); + public IReleaseProbe Inner; + public object SeenProxy; + + public virtual void CallInnerThenCaptureProxy() + { + try + { + Inner.Throw(); + } + catch (InvalidOperationException) + { + } + + SeenProxy = AopContext.CurrentProxy; + } + public bool ThrowFromObjectMethods; public override bool Equals(object obj) => ThrowFromObjectMethods ? throw new InvalidOperationException("boom") : base.Equals(obj); @@ -89,9 +116,21 @@ public object Invoke(AopAlliance.Intercept.IMethodInvocation invocation) } } - private static IReleaseProbe CreateProxy(ITargetSource targetSource, bool decorator, bool advised, AopAlliance.Aop.IAdvice advice = null) + [SetUp] + [TearDown] + public void DrainAopContext() + { + // the proxy stack is thread-static; start and end every test with it empty + while (AopContext.IsActive) + { + AopContext.PopProxy(); + } + } + + private static IReleaseProbe CreateProxy(ITargetSource targetSource, bool decorator, bool advised, AopAlliance.Aop.IAdvice advice = null, bool exposeProxy = false) { ProxyFactory factory = new ProxyFactory(); + factory.ExposeProxy = exposeProxy; factory.TargetSource = targetSource; if (decorator) { @@ -284,4 +323,66 @@ public void PoolIsNotExhaustedByCallsThatThrow(bool decorator, bool advised) pool.Dispose(); } } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void ExposedProxyIsPoppedWhenCallThrows(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised, exposeProxy: true); + + Assert.That(() => proxy.Throw(), Throws.TypeOf()); + Assert.That(AopContext.IsActive, Is.False, "proxy stack leaked an entry after a throwing call"); + AssertBalanced(targetSource, 1); + + Assert.That(() => proxy.ThrowInt(), Throws.TypeOf()); + Assert.That(AopContext.IsActive, Is.False); + + proxy.DoVoid(); + Assert.That(AopContext.IsActive, Is.False); + } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void OuterProxyStaysCurrentAfterInnerCallThrows(bool decorator, bool advised) + { + MockTargetSource innerSource = CreateCountingTargetSource(out _); + IReleaseProbe inner = CreateProxy(innerSource, decorator, advised, exposeProxy: true); + MockTargetSource outerSource = CreateCountingTargetSource(out ReleaseProbe outerProbe); + outerProbe.Inner = inner; + IReleaseProbe outer = CreateProxy(outerSource, decorator, advised, exposeProxy: true); + + outer.CallInnerThenCaptureProxy(); + + Assert.That(outerProbe.SeenProxy, Is.SameAs(outer)); + Assert.That(AopContext.IsActive, Is.False); + AssertBalanced(innerSource, 1); + AssertBalanced(outerSource, 1); + } + + [TestCase(false)] + [TestCase(true)] + public void ExposedProxyIsPoppedWhenIntroducedMethodThrows(bool decorator) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + ProxyFactory factory = new ProxyFactory(); + factory.ExposeProxy = true; + factory.TargetSource = targetSource; + factory.ProxyTargetType = decorator; + if (!decorator) + { + factory.AddInterface(typeof(IReleaseProbe)); + } + + factory.AddIntroduction(new Spring.Aop.Support.DefaultIntroductionAdvisor(new ThrowingIntroduction(), typeof(IThrowingIntroduction))); + IThrowingIntroduction proxy = (IThrowingIntroduction) factory.GetProxy(); + + Assert.That(() => proxy.ThrowIntroduced(), Throws.TypeOf()); + + Assert.That(AopContext.IsActive, Is.False, "proxy stack leaked an entry after a throwing introduced call"); + } } From 3aadb40d2233f02f64f7c9f0fb4e0a04c49f538f Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Wed, 4 Nov 2026 00:46:47 +0200 Subject: [PATCH 3/3] Packet A1: review follow-ups (inheritance and generic coverage, builder docs) Add tests for the inheritance-based proxy with ExposeProxy and a throwing method, and for generic methods (get/release balance and AopContext pop) on composition and decorator proxies. Document the try/finally contract of GenerateMethod/GenerateMethodLogic/EndMethod, and that AbstractAopProxyTypeBuilder.PushTarget borrows without releasing. No behaviour change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LGkLQuAuUjvRjrU1AcTsmk Signed-off-by: Marko Lahma --- .../AbstractAopProxyMethodBuilder.cs | 17 ++++++++- .../AbstractAopProxyTypeBuilder.cs | 7 ++++ .../DynamicProxy/InheritanceAopProxyTests.cs | 35 +++++++++++++++++++ .../DynamicProxy/ReleaseTargetTests.cs | 29 +++++++++++++++ 4 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs index 007f631c7..b0c20fb6f 100644 --- a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs +++ b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyMethodBuilder.cs @@ -178,6 +178,11 @@ protected AbstractAopProxyMethodBuilder( /// /// Generates the proxy method. /// + /// + /// The method logic () is emitted inside a try region whose + /// finally pops the exposed proxy from the stack. An override that + /// bypasses this method must therefore pop the proxy itself, on every path out of the call. + /// /// The IL generator to use. /// The method to proxy. /// @@ -447,6 +452,12 @@ private void PopExposedProxy(ILGenerator il) /// /// Generates method logic. /// + /// + /// The emitted code runs inside try/finally regions (see ), so it must + /// leave the evaluation stack empty, keep the result in the return value local, and must not + /// emit ret 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. + /// /// The IL generator to use. /// The method to proxy. /// @@ -601,8 +612,12 @@ protected abstract void CallDirectProxiedMethod( ILGenerator il, MethodInfo method, MethodInfo interfaceMethod); /// - /// Ends method by returning return value if appropriate. + /// Ends method by loading the return value, if appropriate. /// + /// + /// This runs after the try/finally regions, which have already popped the exposed proxy from + /// the stack. It must not emit ret; the caller does. + /// /// The IL generator to use. /// The method to proxy. protected virtual void EndMethod(ILGenerator il, MethodInfo method) diff --git a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyTypeBuilder.cs b/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyTypeBuilder.cs index f1c851c8b..a9883b6b1 100644 --- a/src/Spring/Spring.Aop/Aop/Framework/DynamicProxy/AbstractAopProxyTypeBuilder.cs +++ b/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. /// + /// + /// This default borrows a target with and emits no matching + /// . 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. + /// /// The IL generator to use. public override void PushTarget(ILGenerator il) { diff --git a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/InheritanceAopProxyTests.cs b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/InheritanceAopProxyTests.cs index decbb62fa..b1df136ff 100644 --- a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/InheritanceAopProxyTests.cs +++ b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/InheritanceAopProxyTests.cs @@ -38,6 +38,41 @@ protected object CreateProxy(AdvisedSupport advisedSupport) return ((IAopProxy) proxyCtorInfo.Invoke(new object[] { advisedSupport })).GetProxy(); } + public class ThrowingInheritanceObject + { + public virtual void Throw() => throw new InvalidOperationException("boom"); + + public virtual void DoNothing() + { + } + } + + [TestCase(false)] + [TestCase(true)] + public void ExposedProxyIsPoppedWhenCallThrows(bool advised) + { + while (AopContext.IsActive) + { + AopContext.PopProxy(); + } + + AdvisedSupport support = new AdvisedSupport(); + support.Target = new ThrowingInheritanceObject(); + support.ExposeProxy = true; + if (advised) + { + support.AddAdvice(new NopInterceptor()); + } + + ThrowingInheritanceObject proxy = (ThrowingInheritanceObject) CreateProxy(support); + + Assert.That(() => proxy.Throw(), Throws.TypeOf()); + Assert.That(AopContext.IsActive, Is.False, "proxy stack leaked an entry after a throwing call"); + + proxy.DoNothing(); + Assert.That(AopContext.IsActive, Is.False); + } + [Test] public void DirectCall() { diff --git a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs index 724afcc00..2f45a5c8f 100644 --- a/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs +++ b/test/Spring/Spring.Aop.Tests/Aop/Framework/DynamicProxy/ReleaseTargetTests.cs @@ -45,6 +45,8 @@ public interface IReleaseProbe ProbeStruct ThrowStruct(); string ThrowString(); void CallInnerThenCaptureProxy(); + T ThrowGeneric(); + T EchoGeneric(T value); } public interface IThrowingIntroduction @@ -83,6 +85,10 @@ public virtual void RefOut(ref int counter, out string text) public virtual string ThrowString() => throw new InvalidOperationException("boom"); + public virtual T ThrowGeneric() => throw new InvalidOperationException("boom"); + + public virtual T EchoGeneric(T value) => value; + public IReleaseProbe Inner; public object SeenProxy; @@ -385,4 +391,27 @@ public void ExposedProxyIsPoppedWhenIntroducedMethodThrows(bool decorator) Assert.That(AopContext.IsActive, Is.False, "proxy stack leaked an entry after a throwing introduced call"); } + + [TestCase(false, false)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(true, true)] + public void GenericMethodsReleaseTargetAndPopExposedProxy(bool decorator, bool advised) + { + MockTargetSource targetSource = CreateCountingTargetSource(out _); + IReleaseProbe proxy = CreateProxy(targetSource, decorator, advised, exposeProxy: true); + + Assert.That(proxy.EchoGeneric(5), Is.EqualTo(5)); + Assert.That(proxy.EchoGeneric("x"), Is.EqualTo("x")); + AssertBalanced(targetSource, 2); + Assert.That(AopContext.IsActive, Is.False); + + Assert.That(() => proxy.ThrowGeneric(), Throws.TypeOf()); + AssertBalanced(targetSource, 3); + Assert.That(AopContext.IsActive, Is.False); + + Assert.That(() => proxy.ThrowGeneric(), Throws.TypeOf()); + AssertBalanced(targetSource, 4); + Assert.That(AopContext.IsActive, Is.False); + } }