From d485f39264ab3e0cecb426a0143717eecbed7305 Mon Sep 17 00:00:00 2001 From: RaymondHuy Date: Thu, 6 Feb 2020 19:54:23 +0700 Subject: [PATCH] Fix Issue#963: Partially decorating service implementing multiple interfaces throws (#1072) * update test case for issue #963 * Add target component to be decorated when applying decorating process * Refactor and remove unused method. --- src/Autofac/Core/ISharingLifetimeScope.cs | 13 +++-- src/Autofac/Core/Lifetime/LifetimeScope.cs | 24 ++++++--- src/Autofac/Core/Resolving/InstanceLookup.cs | 54 +++++++++++-------- .../Features/Decorators/InstanceDecorator.cs | 2 +- src/Autofac/ResolveRequest.cs | 15 ++++++ .../Features/Decorators/DecoratorTests.cs | 15 ++++-- .../Decorators/OpenGenericDecoratorTests.cs | 12 ++--- 7 files changed, 91 insertions(+), 44 deletions(-) diff --git a/src/Autofac/Core/ISharingLifetimeScope.cs b/src/Autofac/Core/ISharingLifetimeScope.cs index 0aee08789..986d478e8 100644 --- a/src/Autofac/Core/ISharingLifetimeScope.cs +++ b/src/Autofac/Core/ISharingLifetimeScope.cs @@ -43,12 +43,19 @@ public interface ISharingLifetimeScope : ILifetimeScope ISharingLifetimeScope? ParentLifetimeScope { get; } /// - /// Try to retrieve an instance based on a GUID key. If the instance - /// does not exist, invoke to create it. + /// Try to retrieve an instance based on a GUID key. /// /// Key to look up. + /// The instance that has the specified key. + /// true if the key was found; otherwise, false. + bool TryGetSharedInstance(Guid id, out object value); + + /// + /// Try to create an instance with a GUID key. + /// + /// Key. /// Creation function. /// An instance. - object GetOrCreateAndShare(Guid id, Func creator); + object CreateSharedInstance(Guid id, Func creator); } } diff --git a/src/Autofac/Core/Lifetime/LifetimeScope.cs b/src/Autofac/Core/Lifetime/LifetimeScope.cs index 9f501940d..b2d4d1320 100644 --- a/src/Autofac/Core/Lifetime/LifetimeScope.cs +++ b/src/Autofac/Core/Lifetime/LifetimeScope.cs @@ -290,30 +290,38 @@ public object ResolveComponent(ResolveRequest request) public ISharingLifetimeScope RootLifetimeScope { get; } /// - /// Try to retrieve an instance based on a GUID key. If the instance - /// does not exist, invoke to create it. + /// Try to create an instance with a GUID key. /// - /// Key to look up. + /// Key. /// Creation function. /// An instance. - public object GetOrCreateAndShare(Guid id, Func creator) + public object CreateSharedInstance(Guid id, Func creator) { if (creator == null) throw new ArgumentNullException(nameof(creator)); - if (_sharedInstances.TryGetValue(id, out var result)) return result; - lock (_synchRoot) { - if (_sharedInstances.TryGetValue(id, out result)) return result; + if (_sharedInstances.TryGetValue(id, out var result)) return result; result = creator(); if (_sharedInstances.ContainsKey(id)) throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, LifetimeScopeResources.SelfConstructingDependencyDetected, result.GetType().FullName)); _sharedInstances.TryAdd(id, result); + + return result; } + } - return result; + /// + /// Try to retrieve an instance based on a GUID key. + /// + /// Key to look up. + /// The instance that has the specified key. + /// true if the key was found; otherwise, false. + public bool TryGetSharedInstance(Guid id, out object value) + { + return _sharedInstances.TryGetValue(id, out value); } /// diff --git a/src/Autofac/Core/Resolving/InstanceLookup.cs b/src/Autofac/Core/Resolving/InstanceLookup.cs index bd78aa781..5b6db72cc 100644 --- a/src/Autofac/Core/Resolving/InstanceLookup.cs +++ b/src/Autofac/Core/Resolving/InstanceLookup.cs @@ -40,6 +40,7 @@ internal class InstanceLookup : IComponentContext, IInstanceLookup { private readonly IResolveOperation _context; private readonly ISharingLifetimeScope _activationScope; + private readonly IComponentRegistration? _decoratorTargetComponent; private readonly Service _service; private object? _newInstance; private bool _executed; @@ -52,6 +53,7 @@ internal class InstanceLookup : IComponentContext, IInstanceLookup { _context = context; _service = request.Service; + _decoratorTargetComponent = request.DecoratorTarget; ComponentRegistration = request.Registration; Parameters = request.Parameters; @@ -81,19 +83,41 @@ public object Execute() _executed = true; object? decoratorTarget = null; - object instance = ComponentRegistration.Sharing == InstanceSharing.None - ? Activate(Parameters, out decoratorTarget) - : _activationScope.GetOrCreateAndShare(ComponentRegistration.Id, () => Activate(Parameters, out decoratorTarget)); + + var sharing = _decoratorTargetComponent != null + ? _decoratorTargetComponent.Sharing + : ComponentRegistration.Sharing; + + var resolveParameters = Parameters as Parameter[] ?? Parameters.ToArray(); + + if (!_activationScope.TryGetSharedInstance(ComponentRegistration.Id, out _newInstance)) + { + _newInstance = sharing == InstanceSharing.Shared + ? _activationScope.CreateSharedInstance(ComponentRegistration.Id, () => CreateInstance(Parameters)) + : CreateInstance(Parameters); + } + + decoratorTarget = _newInstance; + + _newInstance = InstanceDecorator.TryDecorateRegistration( + _service, + ComponentRegistration, + _newInstance, + _activationScope, + resolveParameters); + + if (_newInstance != decoratorTarget) + ComponentRegistration.RaiseActivating(this, resolveParameters, ref _newInstance); var handler = InstanceLookupEnding; handler?.Invoke(this, new InstanceLookupEndingEventArgs(this, NewInstanceActivated)); StartStartableComponent(decoratorTarget); - return instance; + return _newInstance; } - private void StartStartableComponent(object? instance) + private void StartStartableComponent(object instance) { if (instance is IStartable startable && ComponentRegistration.Services.Any(s => (s is TypedService typed) && typed.ServiceType == typeof(IStartable)) @@ -111,7 +135,7 @@ private void StartStartableComponent(object? instance) private bool NewInstanceActivated => _newInstance != null; [SuppressMessage("CA1031", "CA1031", Justification = "General exception gets rethrown in a PropagateActivationException.")] - private object Activate(IEnumerable parameters, out object decoratorTarget) + private object CreateInstance(IEnumerable parameters) { ComponentRegistration.RaisePreparing(this, ref parameters); @@ -119,16 +143,9 @@ private object Activate(IEnumerable parameters, out object decoratorT try { - decoratorTarget = _newInstance = ComponentRegistration.Activator.ActivateInstance(this, resolveParameters); + _newInstance = ComponentRegistration.Activator.ActivateInstance(this, resolveParameters); ComponentRegistration.RaiseActivating(this, resolveParameters, ref _newInstance); - - _newInstance = InstanceDecorator.TryDecorateRegistration( - _service, - ComponentRegistration, - _newInstance, - _activationScope, - resolveParameters); } catch (ObjectDisposedException) { @@ -145,19 +162,16 @@ private object Activate(IEnumerable parameters, out object decoratorT // important. The ProvidedInstanceActivator will NOT dispose of the provided // instance once the instance has been activated - assuming that it will be // done during the lifetime scope's Disposer executing. - if (decoratorTarget is IDisposable instanceAsDisposable) + if (_newInstance is IDisposable instanceAsDisposable) { _activationScope.Disposer.AddInstanceForDisposal(instanceAsDisposable); } - else if (decoratorTarget is IAsyncDisposable asyncDisposableInstance) + else if (_newInstance is IAsyncDisposable asyncDisposableInstance) { _activationScope.Disposer.AddInstanceForAsyncDisposal(asyncDisposableInstance); } } - if (_newInstance != decoratorTarget) - ComponentRegistration.RaiseActivating(this, resolveParameters, ref _newInstance); - return _newInstance; } @@ -185,8 +199,6 @@ public void Complete() var beginningHandler = CompletionBeginning; beginningHandler?.Invoke(this, new InstanceLookupCompletionBeginningEventArgs(this)); - // _newInstance will definitely have been instantiated by the time - // this is called. ComponentRegistration.RaiseActivated(this, Parameters, _newInstance!); var endingHandler = CompletionEnding; diff --git a/src/Autofac/Features/Decorators/InstanceDecorator.cs b/src/Autofac/Features/Decorators/InstanceDecorator.cs index a4e216b49..fa45dc9db 100644 --- a/src/Autofac/Features/Decorators/InstanceDecorator.cs +++ b/src/Autofac/Features/Decorators/InstanceDecorator.cs @@ -69,7 +69,7 @@ internal static class InstanceDecorator invokeParameters[invokeParameters.Length - 2] = serviceParameter; invokeParameters[invokeParameters.Length - 1] = contextParameter; - var resolveRequest = new ResolveRequest(decoratorService, decoratorRegistration, invokeParameters); + var resolveRequest = new ResolveRequest(decoratorService, decoratorRegistration, invokeParameters, registration); instance = context.ResolveComponent(resolveRequest); if (index < decoratorCount - 1) diff --git a/src/Autofac/ResolveRequest.cs b/src/Autofac/ResolveRequest.cs index 99f6595ba..8ede06634 100644 --- a/src/Autofac/ResolveRequest.cs +++ b/src/Autofac/ResolveRequest.cs @@ -15,10 +15,23 @@ public class ResolveRequest /// The component registration for the service. /// The parameters used when resolving the service. public ResolveRequest(Service service, IComponentRegistration registration, IEnumerable parameters) + : this(service, registration, parameters, null) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The service being resolved. + /// The component registration for the service. + /// The parameters used when resolving the service. + /// The target component to be decorated. + public ResolveRequest(Service service, IComponentRegistration registration, IEnumerable parameters, IComponentRegistration? decoratorTarget = null) { Service = service; Registration = registration; Parameters = parameters; + DecoratorTarget = decoratorTarget; } /// @@ -35,5 +48,7 @@ public ResolveRequest(Service service, IComponentRegistration registration, IEnu /// Gets the parameters used when resolving the service. /// public IEnumerable Parameters { get; } + + public IComponentRegistration? DecoratorTarget { get; } } } diff --git a/test/Autofac.Test/Features/Decorators/DecoratorTests.cs b/test/Autofac.Test/Features/Decorators/DecoratorTests.cs index 46ea10a73..31caefbd0 100644 --- a/test/Autofac.Test/Features/Decorators/DecoratorTests.cs +++ b/test/Autofac.Test/Features/Decorators/DecoratorTests.cs @@ -104,7 +104,7 @@ public void DecoratedInstancePerDependencyRegistrationCanIncludeOtherServices() Assert.NotSame(serviceInstance, decoratedServiceInstance); } - [Fact(Skip = "Issue #963")] + [Fact] public void DecoratedInstancePerLifetimeScopeRegistrationCanIncludeOtherServices() { var builder = new ContainerBuilder(); @@ -128,10 +128,10 @@ public void DecoratedInstancePerLifetimeScopeRegistrationCanIncludeOtherServices var decoratedServiceInstance = container.Resolve(); Assert.IsType(decoratedServiceInstance); - Assert.Same(serviceInstance, decoratedServiceInstance); + Assert.Same(serviceInstance, decoratedServiceInstance.Decorated); } - [Fact(Skip = "Issue #963")] + [Fact] public void DecoratedSingleInstanceRegistrationCanIncludeOtherServices() { var builder = new ContainerBuilder(); @@ -149,8 +149,13 @@ public void DecoratedSingleInstanceRegistrationCanIncludeOtherServices() Assert.NotNull(decoratedServiceRegistration); Assert.Same(serviceRegistration, decoratedServiceRegistration); - Assert.IsType(container.Resolve()); - Assert.IsType(container.Resolve()); + var serviceInstance = container.Resolve(); + Assert.IsType(serviceInstance); + + var decoratedServiceInstance = container.Resolve(); + Assert.IsType(decoratedServiceInstance); + + Assert.Same(serviceInstance, decoratedServiceInstance.Decorated); } [Fact] diff --git a/test/Autofac.Test/Features/Decorators/OpenGenericDecoratorTests.cs b/test/Autofac.Test/Features/Decorators/OpenGenericDecoratorTests.cs index bd8b9edf8..68eb787f2 100644 --- a/test/Autofac.Test/Features/Decorators/OpenGenericDecoratorTests.cs +++ b/test/Autofac.Test/Features/Decorators/OpenGenericDecoratorTests.cs @@ -222,7 +222,7 @@ public void DecoratedInstancePerDependencyRegistrationCanIncludeOtherServices() Assert.NotSame(serviceInstance, decoratedServiceInstance); } - [Fact(Skip = "Issue #963")] + [Fact] public void DecoratedInstancePerLifetimeScopeRegistrationCanIncludeOtherServices() { var builder = new ContainerBuilder(); @@ -241,15 +241,15 @@ public void DecoratedInstancePerLifetimeScopeRegistrationCanIncludeOtherServices Assert.Same(serviceRegistration, decoratedServiceRegistration); var serviceInstance = container.Resolve>(); - Assert.IsType>(serviceInstance); + Assert.IsType>(serviceInstance); var decoratedServiceInstance = container.Resolve>(); Assert.IsType>(decoratedServiceInstance); - Assert.Same(serviceInstance, decoratedServiceInstance); + Assert.Same(serviceInstance, decoratedServiceInstance.Decorated); } - [Fact(Skip = "Issue #963")] + [Fact] public void DecoratedSingleInstanceRegistrationCanIncludeOtherServices() { var builder = new ContainerBuilder(); @@ -268,12 +268,12 @@ public void DecoratedSingleInstanceRegistrationCanIncludeOtherServices() Assert.Same(serviceRegistration, decoratedServiceRegistration); var serviceInstance = container.Resolve>(); - Assert.IsType>(serviceInstance); + Assert.IsType>(serviceInstance); var decoratedServiceInstance = container.Resolve>(); Assert.IsType>(decoratedServiceInstance); - Assert.Same(serviceInstance, decoratedServiceInstance); + Assert.Same(serviceInstance, decoratedServiceInstance.Decorated); } [Fact]