From 057a9288f16ff90d315fa3988dbc454597c1a0c9 Mon Sep 17 00:00:00 2001 From: James Skimming Date: Fri, 16 Feb 2018 08:10:38 +0000 Subject: [PATCH 1/3] Added tests to show a race condition when intercepting an async method --- .../AsyncInterceptorTestCase.cs | 48 +++++++++++++++++++ .../Classes/ClassWithAsynchronousMethod.cs | 38 +++++++++++++++ .../IInterfaceWithAsynchronousMethod.cs | 23 +++++++++ .../Interceptors/AsyncInterceptor.cs | 41 ++++++++++++++++ 4 files changed, 150 insertions(+) create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/AsyncInterceptorTestCase.cs create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/Classes/ClassWithAsynchronousMethod.cs create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/Interfaces/IInterfaceWithAsynchronousMethod.cs create mode 100644 src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/AsyncInterceptorTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/AsyncInterceptorTestCase.cs new file mode 100644 index 0000000000..eb190de02a --- /dev/null +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/AsyncInterceptorTestCase.cs @@ -0,0 +1,48 @@ +// Copyright 2004-2016 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace CastleTests.DynamicProxy.Tests +{ + using System; + using System.Linq; + using System.Reflection; + using System.Threading.Tasks; + + using Castle.DynamicProxy; + using Castle.DynamicProxy.Tests; + + using CastleTests.DynamicProxy.Tests.Classes; + using CastleTests.DynamicProxy.Tests.Interfaces; + using CastleTests.Interceptors; + + using NUnit.Framework; + + [TestFixture] + public class AsyncInterceptorTestCase : BasePEVerifyTestCase + { + [Test] + public async Task Should_Intercept_Asynchronous_Methods_With_An_Async_Operations_Prior_To_Calling_Proceed() + { + // Arrange + IInterfaceWithAsynchronousMethod target = new ClassWithAsynchronousMethod(); + IInterceptor interceptor = new AsyncInterceptor(); + + IInterfaceWithAsynchronousMethod proxy = + generator.CreateInterfaceProxyWithTargetInterface(target, interceptor); + + // Act + await proxy.Method().ConfigureAwait(false); + } + } +} diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Classes/ClassWithAsynchronousMethod.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Classes/ClassWithAsynchronousMethod.cs new file mode 100644 index 0000000000..0d7cb439ed --- /dev/null +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Classes/ClassWithAsynchronousMethod.cs @@ -0,0 +1,38 @@ +// Copyright 2004-2010 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace CastleTests.DynamicProxy.Tests.Classes +{ + using System; + using System.Threading; + using System.Threading.Tasks; + + using CastleTests.DynamicProxy.Tests.Interfaces; + + public class ClassWithAsynchronousMethod : IInterfaceWithAsynchronousMethod + { + public async Task Method() + { + Console.WriteLine( + $"Before Await ClassWithAsynchronousMethod:Method ThreadId='{Thread.CurrentThread.ManagedThreadId}'.", + Thread.CurrentThread.ManagedThreadId); + + await Task.Delay(10).ConfigureAwait(false); + + Console.WriteLine( + $"After Await ClassWithAsynchronousMethod:Method ThreadId='{Thread.CurrentThread.ManagedThreadId}'.", + Thread.CurrentThread.ManagedThreadId); + } + } +} diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Interfaces/IInterfaceWithAsynchronousMethod.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Interfaces/IInterfaceWithAsynchronousMethod.cs new file mode 100644 index 0000000000..724bcc5286 --- /dev/null +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Interfaces/IInterfaceWithAsynchronousMethod.cs @@ -0,0 +1,23 @@ +// Copyright 2004-2010 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace CastleTests.DynamicProxy.Tests.Interfaces +{ + using System.Threading.Tasks; + + public interface IInterfaceWithAsynchronousMethod + { + Task Method(); + } +} diff --git a/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs b/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs new file mode 100644 index 0000000000..ea17281ca7 --- /dev/null +++ b/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs @@ -0,0 +1,41 @@ +// Copyright 2004-2010 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace CastleTests.Interceptors +{ + using System.Threading.Tasks; + using Castle.DynamicProxy; + + public class AsyncInterceptor : IInterceptor + { + public void Intercept(IInvocation invocation) + { + invocation.ReturnValue = InterceptAsyncMethod(invocation); + } + + private static async Task InterceptAsyncMethod(IInvocation invocation) + { + // It all falls down when executing async before calling Proceed(). + await Task.Delay(10).ConfigureAwait(false); + + invocation.Proceed(); + + // Hmmmmm, now with it simplified down to this, I see the glaring hole that is the return value being set + // in two situations. + Task returnValue = (Task)invocation.ReturnValue; + + await returnValue.ConfigureAwait(false); + } + } +} From 972bd8c2bd76b5526ac06eaaa2cf88619d2f34a6 Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Mar 2019 23:35:21 +0100 Subject: [PATCH 2/3] Add new `IInvocation.GetMemento` method This is a sneaky way of making `AbstractInvocation.currentInterceptor- Index` modifiable without exposing it. --- .../DynamicProxy/AbstractInvocation.cs | 56 +++++++++++++++++++ src/Castle.Core/DynamicProxy/IInvocation.cs | 6 ++ .../DynamicProxy/IInvocationMemento.cs | 30 ++++++++++ .../DynamicProxy/InvocationMementoOptions.cs | 46 +++++++++++++++ 4 files changed, 138 insertions(+) create mode 100644 src/Castle.Core/DynamicProxy/IInvocationMemento.cs create mode 100644 src/Castle.Core/DynamicProxy/InvocationMementoOptions.cs diff --git a/src/Castle.Core/DynamicProxy/AbstractInvocation.cs b/src/Castle.Core/DynamicProxy/AbstractInvocation.cs index d8d55adf65..a409bee84f 100644 --- a/src/Castle.Core/DynamicProxy/AbstractInvocation.cs +++ b/src/Castle.Core/DynamicProxy/AbstractInvocation.cs @@ -142,6 +142,11 @@ public void Proceed() } } + public IInvocationMemento GetMemento(InvocationMementoOptions options) + { + return new Memento(this, options); + } + protected abstract void InvokeMethodOnTarget(); protected void ThrowOnNoTarget() @@ -188,5 +193,56 @@ private MethodInfo EnsureClosedMethod(MethodInfo method) } return method; } + + private sealed class Memento : IInvocationMemento + { + private readonly AbstractInvocation invocation; + private readonly InvocationMementoOptions options; + + private readonly object[] arguments; + private readonly object returnValue; + private readonly int interceptorIndex; + + public Memento(AbstractInvocation invocation, InvocationMementoOptions options) + { + this.invocation = invocation; + this.options = options; + + if ((options & InvocationMementoOptions.Arguments) != 0) + { + var argumentCount = invocation.arguments.Length; + this.arguments = new object[argumentCount]; + Array.Copy(invocation.arguments, this.arguments, argumentCount); + } + + if ((options & InvocationMementoOptions.ReturnValue) != 0) + { + this.returnValue = invocation.ReturnValue; + } + + if ((options & InvocationMementoOptions.InterceptionProgress) != 0) + { + this.interceptorIndex = invocation.currentInterceptorIndex; + } + } + + public void Restore() + { + if ((options & InvocationMementoOptions.Arguments) != 0) + { + Array.Copy(arguments, invocation.arguments, arguments.Length); + } + + if ((options & InvocationMementoOptions.ReturnValue) != 0) + { + invocation.ReturnValue = returnValue; + } + + if ((options & InvocationMementoOptions.InterceptionProgress) != 0) + { + invocation.currentInterceptorIndex = interceptorIndex; + } + } + } } } \ No newline at end of file diff --git a/src/Castle.Core/DynamicProxy/IInvocation.cs b/src/Castle.Core/DynamicProxy/IInvocation.cs index 35563d3235..76cd78ace5 100644 --- a/src/Castle.Core/DynamicProxy/IInvocation.cs +++ b/src/Castle.Core/DynamicProxy/IInvocation.cs @@ -125,5 +125,11 @@ public interface IInvocation /// The index of the argument to override. /// The new value for the argument. void SetArgumentValue(int index, object value); + + /// + /// Gets an object that captures the specified parts of this invocation's state for later restoration. + /// + /// Specifies which parts of this invocation's state should be captured. + IInvocationMemento GetMemento(InvocationMementoOptions options); } } \ No newline at end of file diff --git a/src/Castle.Core/DynamicProxy/IInvocationMemento.cs b/src/Castle.Core/DynamicProxy/IInvocationMemento.cs new file mode 100644 index 0000000000..048bb1371e --- /dev/null +++ b/src/Castle.Core/DynamicProxy/IInvocationMemento.cs @@ -0,0 +1,30 @@ +// Copyright 2004-2019 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace Castle.DynamicProxy +{ + using System.ComponentModel; + + /// + /// Represents a full or partial state of an captured at an earlier time. + /// + [EditorBrowsable(EditorBrowsableState.Advanced)] + public interface IInvocationMemento + { + /// + /// Restores the state captured by this instance to the associated . + /// + void Restore(); + } +} diff --git a/src/Castle.Core/DynamicProxy/InvocationMementoOptions.cs b/src/Castle.Core/DynamicProxy/InvocationMementoOptions.cs new file mode 100644 index 0000000000..e3522b4e0d --- /dev/null +++ b/src/Castle.Core/DynamicProxy/InvocationMementoOptions.cs @@ -0,0 +1,46 @@ +// Copyright 2004-2019 Castle Project - http://www.castleproject.org/ +// +// 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. + +namespace Castle.DynamicProxy +{ + using System; + + /// + /// Specifies which parts of an invocation's state to capture. + /// + [Flags] + public enum InvocationMementoOptions + { + /// + /// Specifies that arguments should be captured. + /// + Arguments = 1, + + /// + /// Specifies that the return value should be captured. + /// + ReturnValue = 2, + + /// + /// Specifies that the invocation's position in the interception pipeline should be captured. + /// + /// Note that interception of an invocation may have finished by the time a call to + /// is made. While such a "stale" invocation + /// can be procesed again by interceptors, setting the return value or by-ref arguments + /// will no longer have any effect on the calling code. + /// + /// + InterceptionProgress = 4, + } +} From 6242cdc4a60c8721141bd9f1008a8eb880167a6a Mon Sep 17 00:00:00 2001 From: stakx Date: Sat, 23 Mar 2019 23:37:57 +0100 Subject: [PATCH 3/3] Fix async interception test --- src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs b/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs index ea17281ca7..1609bae0ec 100644 --- a/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs +++ b/src/Castle.Core.Tests/Interceptors/AsyncInterceptor.cs @@ -26,9 +26,13 @@ public void Intercept(IInvocation invocation) private static async Task InterceptAsyncMethod(IInvocation invocation) { + var interceptionProgress = invocation.GetMemento(InvocationMementoOptions.InterceptionProgress); + // It all falls down when executing async before calling Proceed(). await Task.Delay(10).ConfigureAwait(false); + interceptionProgress.Restore(); + invocation.Proceed(); // Hmmmmm, now with it simplified down to this, I see the glaring hole that is the return value being set