diff --git a/src/SimpleInjector.Tests.Unit/ScopeTests.cs b/src/SimpleInjector.Tests.Unit/ScopeTests.cs index f5d50721f..f01907ffd 100644 --- a/src/SimpleInjector.Tests.Unit/ScopeTests.cs +++ b/src/SimpleInjector.Tests.Unit/ScopeTests.cs @@ -1,6 +1,7 @@ namespace SimpleInjector.Tests.Unit { using System; + using System.Linq; using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; using SimpleInjector.Lifestyles; @@ -349,7 +350,7 @@ public void Dispose_WithAsyncDisposable_ThrowsException() // Assert AssertThat.ThrowsWithExceptionMessageContains( "AsyncDisposable only implements IAsyncDisposable, but not IDisposable. Make sure to call " + - "Scope.DisposeScopeAsync() instead of Dispose().", + "Scope.DisposeScopeAsync() instead of Scope.Dispose().", action); } @@ -420,6 +421,68 @@ public async Task GetDisposables_WithSyncAndAsyncDisposableInstances_OnlyReturns await scope.DisposeScopeAsync(); } + [TestMethod] + public void Dispose_ContainerAndAysncDisposableResolvedAsSingletons_ThrowsExpectedException() + { + // Arrange + var container = new Container(); + + container.Register(Lifestyle.Singleton); + + // It might seem odd to test especially in the context of having the Container resolved as + // dependency, but there was actually a bug with that particular case that caused Dispose to throw + // an ObjectDisposedException (oh the irony), instead of the correct InvalidOperationException. + container.Register>(Lifestyle.Singleton); + + container.Verify(); + + // Act + Action action = () => container.Dispose(); + + // Assert + AssertThat.ThrowsWithExceptionMessageContains( + expectedMessage: "AsyncDisposable only implements IAsyncDisposable, but not IDisposable. " + + "Make sure to call Container.DisposeScopeAsync() instead of Container.Dispose().", + action); + } + + [TestMethod] + public void ContainerScopeGetDisposables_WithContainerResolvedAsDependency_DoesNotContainTheContainer() + { + // Arrange + var container = new Container(); + + // It would be awkward if the container would hold a reference to itself for disposal. + container.Register>(Lifestyle.Singleton); + + // Act + container.Verify(); + + // Assert + Assert.IsFalse(container.ContainerScope.GetAllDisposables().Contains(container)); + Assert.IsFalse(container.ContainerScope.GetDisposables().Contains(container)); + } + + [TestMethod] + public void GetDisposables_WithScopeResolvedAsDependency_DoesNotContainTheScope() + { + // Arrange + var container = new Container(); + container.Options.DefaultScopedLifestyle = ScopedLifestyle.Flowing; + + // It would be awkward if the scope would hold a reference to itself for disposal. + container.Register>(Lifestyle.Scoped); + + var scope = new Scope(container); + + // Act + scope.GetInstance>(); + + // Assert + Assert.IsFalse(scope.GetAllDisposables().Contains(container)); + Assert.IsFalse(scope.GetDisposables().Contains(container)); + } + public interface IFoo { } public class NonDisposable : IFoo { } diff --git a/src/SimpleInjector/Container.Common.cs b/src/SimpleInjector/Container.Common.cs index 2fdc3be18..c388eda6d 100644 --- a/src/SimpleInjector/Container.Common.cs +++ b/src/SimpleInjector/Container.Common.cs @@ -183,6 +183,8 @@ public bool IsLocked internal bool HasResolveInterceptors => this.resolveInterceptors.Count > 0; + internal bool IsDisposed => this.disposed; + /// /// Returns an array with the current registrations. This list contains all explicitly registered /// types, and all implicitly registered instances. Implicit registrations are all concrete diff --git a/src/SimpleInjector/ContainerScope.cs b/src/SimpleInjector/ContainerScope.cs index 6c85bfd23..23e2a987a 100644 --- a/src/SimpleInjector/ContainerScope.cs +++ b/src/SimpleInjector/ContainerScope.cs @@ -20,7 +20,8 @@ public partial class ContainerScope : ApiObject { private readonly Scope scope; - internal ContainerScope(Container container) => this.scope = new Scope(container); + internal ContainerScope(Container container) => + this.scope = new Scope(container) { IsContainerScope = true }; /// /// Allows registering an delegate that will be called when the container @@ -131,10 +132,25 @@ public T GetOrSetItem(object key, Func valueFactory) /// disposed in opposite order as they appear in the list. /// /// The list of instances that will be disposed of when thi - /// + /// /// instance is being disposed. public IDisposable[] GetDisposables() => this.scope.GetDisposables(); + /// + /// Returns a copy of the list of IDisposable and IAsyncDisposable instances that will be disposed of + /// when this instance is being disposed. The list contains scoped instances that + /// are cached in this instance, and instances explicitly registered for + /// disposal using . The instances are returned in order of + /// creation. + /// + /// + /// Thread safety: This method is not thread safe and should not be used in combination + /// with any of the thread-safe methods. + /// + /// The list of instances that will be disposed of when this + /// instance is being disposed. + public object[] GetAllDisposables() => this.scope.GetAllDisposables(); + // This method is internal, because we don't want to expose Dispose through its public API. That would // allow the container scope to be disposed, while the container isn't. Disposing should happen using the // Container API. diff --git a/src/SimpleInjector/ProducerBuilders/UnregisteredTypeResolutionInstanceProducerBuilder.cs b/src/SimpleInjector/ProducerBuilders/UnregisteredTypeResolutionInstanceProducerBuilder.cs index 843a66765..39f3e130f 100644 --- a/src/SimpleInjector/ProducerBuilders/UnregisteredTypeResolutionInstanceProducerBuilder.cs +++ b/src/SimpleInjector/ProducerBuilders/UnregisteredTypeResolutionInstanceProducerBuilder.cs @@ -37,7 +37,8 @@ internal UnregisteredTypeResolutionInstanceProducerBuilder( () => scopeLifestyle.CreateProducer(() => scopeLifestyle.GetCurrentScope(container), container)); this.resolveUnregisteredTypeRegistrations[typeof(Container)] = new LazyEx( - () => Lifestyle.Singleton.CreateProducer(() => container, container)); + () => new InstanceProducer(typeof(Container), + Lifestyle.Singleton.CreateRegistration(typeof(Container), container, container))); } // Instead of wrapping the complete method in a lock, we lock inside the individual methods. We diff --git a/src/SimpleInjector/Scope.cs b/src/SimpleInjector/Scope.cs index 3bf6d524c..d685503e8 100644 --- a/src/SimpleInjector/Scope.cs +++ b/src/SimpleInjector/Scope.cs @@ -107,6 +107,8 @@ private enum DisposeState internal Scope? ParentScope { get; } + internal bool IsContainerScope { get; set; } + /// Gets an instance of the given for the current scope. /// The type of the service to resolve. /// Thread safety: Calls to this method are thread safe. @@ -375,7 +377,7 @@ public object[] GetAllDisposables() return Helpers.Array.Empty; } else - { + { var list = this.disposables.ToArray(); for (int index = 0; index < list.Length; index++) @@ -758,22 +760,31 @@ private void DisposeInstancesInReverseOrder( private void ThrowTypeOnlyImplementsIAsyncDisposable(object instance) { - // In case there is no active (ambient) scope, Verify creates and disposes of its own - // scope, but Verify() is completely synchronous and creating a VerifyAsync makes - // little sense because: - // 1. in many cases the user will call Verify() in a context where there is no option - // to await (ASP.NET Core startup, ASP.NET MVC startup, etc). - // 2. Verify is called during a call to GetInstance when auto verification is enabled. - // Adding VerifyAsync() would, therefore, mena adding an GetInstanceAsync(), but - // would be a bad to add such method (see: https://stackoverflow.com/a/43240576/). - // At this point, we can either: - // 1. ignore the instance with the risk of causing a memory leak - // 2. call DisposeAsync().GetAwaiter().GetResult() with the possible risk of deadlock. - // Because 2 a really, really bad place to be in, we pick 1 and ignore it. - if (this.Container?.IsVerifying != true) - { - throw new InvalidOperationException( - StringResources.TypeOnlyImplementsIAsyncDisposable(instance)); + // Must first check for disposal because IsVerifying throws when the container is disposed of. + if (this.Container?.IsDisposed == true) + { + // In case there is no active (ambient) scope, Verify creates and disposes of its own scope. + // Verify(), however, is completely synchronous and add a Container.VerifyAsync() method makes + // little sense because: + // 1. in many cases the user will call Verify() in a context where there is no option + // to await (ASP.NET Core startup, ASP.NET MVC startup, etc). + // 2. Verify is called during a call to GetInstance when auto verification is enabled. + // Adding VerifyAsync() would, therefore, mena adding an GetInstanceAsync(), but + // would be a bad to add such method (see: https://stackoverflow.com/a/43240576/). + // At this point, we can either: + // 1. ignore the instance with the risk of causing a memory leak + // 2. call DisposeAsync().GetAwaiter().GetResult() with the possible risk of deadlock. + // Because 2 a really, really bad place to be in, we pick 1 and choose to ignore those async + // disposables during verification. If this is a problem the user can two things to prevent + // those disposables from _not_ being disposed: + // 1. Wrap the call to Verify() in a Scope + // 2. Skip the call to Verify() and rely on auto verification, which will typically be + // executed within the context of an active scope. + if (this.Container?.IsVerifying != true) + { + throw new InvalidOperationException( + StringResources.TypeOnlyImplementsIAsyncDisposable(instance, this.IsContainerScope)); + } } } diff --git a/src/SimpleInjector/StringResources.cs b/src/SimpleInjector/StringResources.cs index 096e6b413..3cfaa10e9 100644 --- a/src/SimpleInjector/StringResources.cs +++ b/src/SimpleInjector/StringResources.cs @@ -993,7 +993,7 @@ internal static string UnableToLoadTypesFromAssembly(Assembly assembly, Exceptio assembly.FullName, innerException.Message); - internal static string TypeOnlyImplementsIAsyncDisposable(object instance) + internal static string TypeOnlyImplementsIAsyncDisposable(object instance, bool containerScope) { string asyncDisposeMethod = #if NETSTANDARD2_1 @@ -1004,12 +1004,12 @@ internal static string TypeOnlyImplementsIAsyncDisposable(object instance) return Format( "{0} only implements IAsyncDisposable, but not IDisposable. " + - "Make sure to call Scope.{1}() instead of Dispose().", + "Make sure to call {1}.{2}() instead of {1}.Dispose().", instance.GetType().TypeName(), + containerScope ? nameof(Container) : nameof(Scope), asyncDisposeMethod); } - private static bool IsListOrArrayRelationship(KnownRelationship relationship) => typeof(List<>).IsGenericTypeDefinitionOf(relationship.Consumer.Target.TargetType) || relationship.Consumer.Target.TargetType.IsArray;