diff --git a/Changelog.md b/Changelog.md index 44befd2..9c803eb 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,10 +4,14 @@ ### Breaking Changes +- Open-generic startup warnings now use the root `ILoggerFactory` rather than a directly registered `ILogger`. Configure standard logging (for example, `AddLogging`) to receive them. Warning-delivery failures no longer fail provider creation; warnings are best-effort [#90](https://github.com/PrimordialCode/Mammoth.Extensions.DependencyInjection/issues/90). + - Raise `Microsoft.Extensions.DependencyInjection` to a minimum of **10.0.0**, including the upstream keyed enumerable/open-generic cache identity fix ([dotnet/runtime#113343](https://github.com/dotnet/runtime/pull/113343), [#46](https://github.com/PrimordialCode/Mammoth.Extensions.DependencyInjection/issues/46)). Raise its required `Microsoft.Bcl.AsyncInterfaces` dependency to **10.0.0**. Consumers pinning DI 8.x/9.x must upgrade; all existing library target frameworks remain supported. ### Bug Fixes +- Remove the temporary startup-warning scope and return the provider even when optional warning delivery fails, preserving ownership of logging dependencies until provider disposal and avoiding forced synchronous disposal of async-only scoped loggers [#90](https://github.com/PrimordialCode/Mammoth.Extensions.DependencyInjection/issues/90). + - Preserve native DI constructor preference and parameter-type ambiguity rules when transient-disposable diagnostics instrument ordinary type registrations, including keyed context, optional defaults, null factory results and disposal ownership [#88](https://github.com/PrimordialCode/Mammoth.Extensions.DependencyInjection/issues/88). - Build provider metadata on a private collection copy so repeated or failed builds leave caller registrations unchanged, avoid accumulating support services, and keep each provider's discovery and compatibility metadata independent [#64](https://github.com/PrimordialCode/Mammoth.Extensions.DependencyInjection/issues/64). diff --git a/README.md b/README.md index 0f3df0b..0b8803c 100644 --- a/README.md +++ b/README.md @@ -296,9 +296,11 @@ Instead of re-implementing a new ServiceProvider from scratch, this approach mod Options: - AllowSingletonToResolveTransientDisposables: Defaults to false. If true, permits the transient when the tracked ancestor chain contains a singleton. Resolution frames are isolated between execution-context branches. A task inherits the ancestry captured when it is scheduled, even if it outlives the originating factory; that inherited singleton ancestor still grants this exemption. -- ThrowOnOpenGenericTransientDisposable: Rejects recognized disposable open-generic type registrations at build time, naming the implementation type in the exception for both keyed and unkeyed registrations. Applies to IDisposable and IAsyncDisposable implementations. When false, these registrations instead produce warnings if a logger is available. +- ThrowOnOpenGenericTransientDisposable: Rejects recognized disposable open-generic type registrations at build time, naming the implementation type in the exception for both keyed and unkeyed registrations. Applies to IDisposable and IAsyncDisposable implementations. When false, these registrations instead produce best-effort warnings through the root ILoggerFactory, if registered (normally via AddLogging). - DetectIncorrectUsageOfTransientDisposablesExclusionPatterns: list of Regex patterns to exclude services from detection, matching registrations bypass the diagnostic check. All diagnostic flags default to false. Factory-created disposable objects can exist before a diagnostic throws; normal scope disposal remains necessary. +Open-generic startup warnings use the `Mammoth.Extensions.DependencyInjection.ServiceProviderFactory` logging category and event ID 1. They do not create a temporary scope or resolve `ILogger` from DI. A direct registration of that logger alone no longer supplies these warnings; configure standard `ILoggerFactory` logging instead. Failures while resolving the logging factory, creating the logger or writing a warning are ignored, so a warning may be lost. Provider construction, validation and strict open-generic rejection still propagate their errors. Services created while attempting warning delivery remain owned by the returned provider; dispose it normally, using `DisposeAsync` when its services require asynchronous disposal. + ###### IsRegistered extension methods Additional methods for `IServiceProvider`: diff --git a/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposables.OpenGeneric.Tests.cs b/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposables.OpenGeneric.Tests.cs index 69ee344..2686953 100644 --- a/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposables.OpenGeneric.Tests.cs +++ b/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposables.OpenGeneric.Tests.cs @@ -1,6 +1,5 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Testing; using System.Text.RegularExpressions; namespace Mammoth.Extensions.DependencyInjection.Tests; @@ -35,19 +34,19 @@ public void RejectionNamesTheImplementation(bool keyed, Type implementationType) public async Task WarningModeNamesTheImplementationAndAllowsResolution(bool keyed, Type implementationType) { var services = CreateServices(keyed, implementationType, ServiceLifetime.Transient); - var logger = new FakeLogger(); - services.AddSingleton>(logger); + services.AddLogging(builder => builder.AddFakeLogging()); var options = CreateOptions(); options.ThrowOnOpenGenericTransientDisposable = false; await using var provider = ServiceProviderFactory.CreateServiceProvider(services, options); - Assert.AreEqual(1, logger.Collector.Count); - Assert.AreEqual(LogLevel.Warning, logger.LatestRecord.Level); + var collector = provider.GetFakeLogCollector(); + Assert.AreEqual(1, collector.Count); + Assert.AreEqual(LogLevel.Warning, collector.LatestRecord.Level); Assert.AreEqual( $"Open generic transient disposable registration detected, ServiceKey: {(keyed ? "resource" : "(null)")}, " + $"ServiceType: {typeof(IResource<>)}, ImplementationType: {implementationType}", - logger.LatestRecord.Message); + collector.LatestRecord.Message); await AssertResolution(provider, keyed, implementationType, ServiceLifetime.Transient); } diff --git a/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposablesTests.cs b/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposablesTests.cs index 9faef85..dfbf8cd 100644 --- a/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposablesTests.cs +++ b/src/Mammoth.Extensions.DependencyInjection.Tests/DetectIncorrectUsageOfTransientDisposablesTests.cs @@ -4,7 +4,6 @@ using Mammoth.Extensions.DependencyInjection.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Testing; namespace Mammoth.Extensions.DependencyInjection.Tests { @@ -629,9 +628,7 @@ public void Register_OpenGeneric_TransientDisposable_WithValidation_Throws() public void Register_OpenGeneric_TransientDisposable_WithoutValidation_LogsError() { var serviceCollection = CreateServiceCollection(); - // register a fake logger - var fakeLogger = new FakeLogger(); - serviceCollection.AddSingleton>(fakeLogger); + serviceCollection.AddLogging(builder => builder.AddFakeLogging()); using var sp = ServiceProviderFactory.CreateServiceProvider(serviceCollection, new ExtendedServiceProviderOptions { @@ -640,9 +637,10 @@ public void Register_OpenGeneric_TransientDisposable_WithoutValidation_LogsError ValidateOnBuild = true, ValidateScopes = true }); - Assert.AreEqual(1, fakeLogger.Collector.Count); - Assert.AreEqual(LogLevel.Warning, fakeLogger.LatestRecord.Level); - Assert.AreEqual("Open generic transient disposable registration detected, ServiceKey: (null), ServiceType: Mammoth.Extensions.DependencyInjection.Tests.DetectIncorrectUsageOfTransientDisposablesTests+ITransientOpenGeneric`1[T], ImplementationType: Mammoth.Extensions.DependencyInjection.Tests.DetectIncorrectUsageOfTransientDisposablesTests+TransientOpenGeneric`1[T]", fakeLogger.LatestRecord.Message); + var collector = sp.GetFakeLogCollector(); + Assert.AreEqual(1, collector.Count); + Assert.AreEqual(LogLevel.Warning, collector.LatestRecord.Level); + Assert.AreEqual("Open generic transient disposable registration detected, ServiceKey: (null), ServiceType: Mammoth.Extensions.DependencyInjection.Tests.DetectIncorrectUsageOfTransientDisposablesTests+ITransientOpenGeneric`1[T], ImplementationType: Mammoth.Extensions.DependencyInjection.Tests.DetectIncorrectUsageOfTransientDisposablesTests+TransientOpenGeneric`1[T]", collector.LatestRecord.Message); } [TestMethod] diff --git a/src/Mammoth.Extensions.DependencyInjection.Tests/ServiceProviderFactory.WarningCleanup.Tests.cs b/src/Mammoth.Extensions.DependencyInjection.Tests/ServiceProviderFactory.WarningCleanup.Tests.cs new file mode 100644 index 0000000..10394eb --- /dev/null +++ b/src/Mammoth.Extensions.DependencyInjection.Tests/ServiceProviderFactory.WarningCleanup.Tests.cs @@ -0,0 +1,338 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using System.Text.RegularExpressions; + +namespace Mammoth.Extensions.DependencyInjection.Tests; + +[TestClass] +public class ServiceProviderFactoryWarningCleanupTests +{ + public static IEnumerable WarningFailureCases() + { + foreach (var diagnostics in new[] { false, true }) + foreach (var failure in new[] { "resolve", "create", "enabled", "log" }) + foreach (var disposal in new[] { "sync", "async", "dual" }) + foreach (var disposeAsync in new[] { false, true }) + { + if (disposal != "async" || disposeAsync) + yield return [diagnostics, failure, disposal, disposeAsync]; + } + } + + [TestMethod] + [DynamicData(nameof(WarningFailureCases))] + public async Task WarningFailureReturnsProviderAndRetainsNativeOwnership( + bool diagnostics, string failure, string disposal, bool disposeAsync) + { + var resource = CreateResource(disposal); + var callerOwned = new SyncResource(); + var expected = new InvalidOperationException("warning " + failure + " failed"); + var factory = new RecordingLoggerFactory(failure, expected); + var services = CreateServices(); + var activations = 0; + services.AddSingleton(typeof(ILogger<>), typeof(Logger<>)); + services.AddSingleton(_ => resource); + services.AddKeyedSingleton("caller", callerOwned); + services.AddSingleton(provider => + { + activations++; + provider.GetRequiredService(); + provider.GetRequiredKeyedService("caller"); + if (failure == "resolve") + throw expected; + return factory; + }); + + var provider = diagnostics ? Build(services) : services.BuildServiceProvider(); + try + { + if (!diagnostics) + { + var actual = Assert.ThrowsExactly(() => + { + var logger = provider.GetRequiredService().CreateLogger("control"); + if (logger.IsEnabled(LogLevel.Warning)) + logger.LogWarning("control"); + }); + Assert.AreSame(expected, actual); + } + Assert.AreEqual(1, activations); + Assert.AreEqual(failure == "resolve" ? 0 : 1, factory.CreateCalls); + Assert.AreEqual(failure is "enabled" or "log" ? 1 : 0, factory.Logger.EnabledCalls); + Assert.AreEqual(failure == "log" ? 1 : 0, factory.Logger.LogCalls); + Assert.AreEqual(0, resource.Disposals); + Assert.AreEqual(0, factory.Disposals); + Assert.IsNotNull(provider.GetRequiredService()); + } + finally + { + if (disposeAsync) + await provider.DisposeAsync(); + else + provider.Dispose(); + } + + Assert.AreEqual(1, resource.Disposals); + Assert.AreEqual(disposeAsync && disposal != "sync" ? 1 : 0, resource.AsyncDisposals); + Assert.AreEqual(failure == "resolve" ? 0 : 1, factory.Disposals); + Assert.AreEqual(0, callerOwned.Disposals); + // The returned provider retains native idempotent disposal behavior. + await provider.DisposeAsync(); + provider.Dispose(); + Assert.AreEqual(1, resource.Disposals); + Assert.AreEqual(failure == "resolve" ? 0 : 1, factory.Disposals); + } + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task StandardLoggingEmitsKeyedAndUnkeyedWarningsWithoutActivatingScopedLogger(bool validateOnBuild) + { + var services = CreateServices(); + services.AddKeyedTransient(typeof(OpenResource<>), "key"); + services.AddLogging(builder => builder.AddFakeLogging()); + var logger = new AsyncLogger(); + var loggerActivations = 0; + services.AddScoped>(_ => + { + loggerActivations++; + return logger; + }); + var options = CreateOptions(); + options.ValidateOnBuild = validateOnBuild; + + await using var provider = Build(services, options); + var records = provider.GetFakeLogCollector().GetSnapshot(); + Assert.AreEqual(2, records.Count); + Assert.AreEqual(0, loggerActivations); + Assert.AreEqual(0, logger.Disposals); + foreach (var record in records) + { + Assert.AreEqual(typeof(ServiceProviderFactory).FullName, record.Category); + Assert.AreEqual(LogLevel.Warning, record.Level); + Assert.AreEqual(1, record.Id.Id); + StringAssert.Contains(record.Message, "ServiceType: " + typeof(OpenResource<>)); + StringAssert.Contains(record.Message, "ImplementationType: " + typeof(OpenResource<>)); + } + StringAssert.Contains(records[0].Message, "ServiceKey: (null)"); + StringAssert.Contains(records[1].Message, "ServiceKey: key"); + + // The custom scoped registration remains usable by its actual consumer. + await using (var scope = provider.CreateAsyncScope()) + Assert.AreSame(logger, scope.ServiceProvider.GetRequiredService>()); + Assert.AreEqual(1, loggerActivations); + Assert.AreEqual(1, logger.Disposals); + } + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task MissingLoggerFactoryDoesNotActivateDirectLogger(bool throwFromFactory) + { + var services = CreateServices(); + var logger = new AsyncLogger(); + var activations = 0; + services.AddScoped>(_ => + { + activations++; + if (throwFromFactory) + throw new InvalidOperationException("direct logger must not be activated"); + return logger; + }); + + await using var provider = Build(services); + Assert.AreEqual(0, activations); + Assert.AreEqual(0, logger.Disposals); + Assert.IsNotNull(provider.GetRequiredService()); + } + + [TestMethod] + [DataRow("disabled")] + [DataRow("no-options")] + [DataRow("no-warning")] + [DataRow("excluded")] + [DataRow("strict")] + public void WarningControlsDoNotActivateLoggerFactory(string mode) + { + var services = CreateServices(); + var activations = 0; + services.AddSingleton(_ => + { + activations++; + throw new InvalidOperationException("logger factory must not be activated"); + }); + var options = CreateOptions(); + if (mode == "disabled") + options.DetectIncorrectUsageOfTransientDisposables = false; + if (mode == "no-warning") + services.Remove(services.First(descriptor => descriptor.ServiceType == typeof(OpenResource<>))); + if (mode == "excluded") + options.DetectIncorrectUsageOfTransientDisposablesExclusionPatterns = + ["^" + Regex.Escape(typeof(OpenResource<>).FullName!) + "$"]; + if (mode == "strict") + { + options.ThrowOnOpenGenericTransientDisposable = true; + var error = Assert.ThrowsExactly(() => Build(services, options)); + StringAssert.Contains(error.Message, "Trying to register an open generic transient disposable service"); + } + else + { + using var provider = mode == "no-options" + ? ServiceProviderFactory.CreateServiceProvider(services) + : Build(services, options); + } + Assert.AreEqual(0, activations); + } + + [TestMethod] + public void BuildValidationFailureStillPropagates() + { + var services = CreateServices(); + services.AddSingleton(); + var options = CreateOptions(); + options.ValidateOnBuild = true; + + var error = Assert.ThrowsExactly(() => Build(services, options)); + StringAssert.Contains(error.ToString(), nameof(IMissing)); + } + + [TestMethod] + public void LoggerFactoryAndCallerInstancesRetainTheirOriginalOwnership() + { + var factory = new RecordingLoggerFactory(); + var services = CreateServices(); + services.AddSingleton(factory); + using (var provider = Build(services)) + { + Assert.AreEqual(1, factory.CreateCalls); + Assert.AreEqual(1, factory.Logger.Warnings); + Assert.AreEqual(0, factory.Disposals); + } + Assert.AreEqual(0, factory.Disposals); + } + + [TestMethod] + public void WarningFailureDoesNotSwallowLaterProviderDisposalFailure() + { + var expected = new InvalidOperationException("resource disposal failed"); + var resource = new ThrowingResource(expected); + var services = CreateServices(); + services.AddSingleton(_ => resource); + services.AddSingleton(provider => + { + provider.GetRequiredService(); + throw new InvalidOperationException("logger factory failed"); + }); + var provider = Build(services); + + Assert.AreSame(expected, Assert.ThrowsExactly(provider.Dispose)); + Assert.AreEqual(1, resource.Disposals); + } + + private static IServiceCollection CreateServices() + { + var services = new ServiceCollection(); + services.AddTransient(typeof(OpenResource<>)); + // Native controls need the support type supplied explicitly. + services.AddSingleton(new ServiceTypes()); + return services; + } + + private static ExtendedServiceProviderOptions CreateOptions() => new() + { + DetectIncorrectUsageOfTransientDisposables = true, + ThrowOnOpenGenericTransientDisposable = false, + ValidateOnBuild = false, + ValidateScopes = true + }; + + private static ServiceProvider Build(IServiceCollection services, ExtendedServiceProviderOptions? options = null) => + ServiceProviderFactory.CreateServiceProvider(services, options ?? CreateOptions()); + + private static Resource CreateResource(string disposal) => disposal switch + { + "sync" => new SyncResource(), + "async" => new AsyncResource(), + _ => new DualResource() + }; + + public sealed class OpenResource : IDisposable { public void Dispose() { } } + public interface IMissing; + public sealed class MissingDependency(IMissing missing) { public IMissing Missing { get; } = missing; } + public abstract class Resource + { + public int Disposals; + public int AsyncDisposals; + } + public sealed class SyncResource : Resource, IDisposable + { + public void Dispose() => Disposals++; + } + public sealed class AsyncResource : Resource, IAsyncDisposable + { + public async ValueTask DisposeAsync() + { + await Task.Yield(); + Disposals++; + AsyncDisposals++; + } + } + public sealed class DualResource : Resource, IDisposable, IAsyncDisposable + { + public void Dispose() => Disposals++; + public async ValueTask DisposeAsync() + { + await Task.Yield(); + Disposals++; + AsyncDisposals++; + } + } + public sealed class ThrowingResource(Exception error) : IDisposable + { + public int Disposals; + public void Dispose() { Disposals++; throw error; } + } + + public class RecordingLogger(string? failure = null, Exception? error = null) : ILogger + { + public int Warnings; + public int EnabledCalls; + public int LogCalls; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) + { + EnabledCalls++; + if (failure == "enabled") + throw error!; + return true; + } + public void Log(LogLevel logLevel, EventId eventId, TState state, + Exception? exception, Func formatter) + { + LogCalls++; + if (failure == "log") + throw error!; + Warnings++; + } + } + public sealed class AsyncLogger : RecordingLogger, IAsyncDisposable + { + public int Disposals; + public async ValueTask DisposeAsync() { await Task.Yield(); Disposals++; } + } + public sealed class RecordingLoggerFactory(string? failure = null, Exception? error = null) : ILoggerFactory + { + public readonly RecordingLogger Logger = new(failure, error); + public int CreateCalls; + public int Disposals; + public ILogger CreateLogger(string categoryName) + { + CreateCalls++; + if (failure == "create") + throw error!; + return Logger; + } + public void AddProvider(ILoggerProvider provider) => throw new NotSupportedException(); + public void Dispose() => Disposals++; + } +} diff --git a/src/Mammoth.Extensions.DependencyInjection/ExtendedServiceProviderOptions.cs b/src/Mammoth.Extensions.DependencyInjection/ExtendedServiceProviderOptions.cs index e2f26f8..662c631 100644 --- a/src/Mammoth.Extensions.DependencyInjection/ExtendedServiceProviderOptions.cs +++ b/src/Mammoth.Extensions.DependencyInjection/ExtendedServiceProviderOptions.cs @@ -30,6 +30,9 @@ public class ExtendedServiceProviderOptions : ServiceProviderOptions /// /// Throw an exception if a open generic transient disposable is registered. /// Open Generic cannot be tracked, so it's better to use only closed type registrations. + /// When false, warnings use the root ILoggerFactory, if registered. Warning delivery is best-effort: + /// logger activation or logging failures do not prevent returning the provider. + /// Direct ILogger<ServiceProviderFactory> registrations are not used for these warnings. /// This setting is only relevant if is set to true. /// public bool ThrowOnOpenGenericTransientDisposable { get; set; } diff --git a/src/Mammoth.Extensions.DependencyInjection/ServiceProviderFactory.cs b/src/Mammoth.Extensions.DependencyInjection/ServiceProviderFactory.cs index f88d571..d9086f2 100644 --- a/src/Mammoth.Extensions.DependencyInjection/ServiceProviderFactory.cs +++ b/src/Mammoth.Extensions.DependencyInjection/ServiceProviderFactory.cs @@ -77,23 +77,30 @@ public static ServiceProvider CreateServiceProvider(IServiceCollection container options.DetectIncorrectUsageOfTransientDisposablesExclusionPatterns ); sc = DetectIncorrectUsageOfTransientDisposables.PatchForResolutionContextTracking(patchedSc); - //// return new ResolutionContextTrackingServiceProviderDecorator(sc.BuildServiceProvider(options)); var sp = sc.BuildServiceProvider(options); if (openGenerics.Count > 0 && !options.ThrowOnOpenGenericTransientDisposable) { - // log warning for open generic registration - using var score = sp.CreateScope(); - var logger = score.ServiceProvider.GetService>(); - if (logger != null) + try { - foreach (var openGeneric in openGenerics) + // Use standard logging without creating a temporary scope. Any services + // activated here remain owned by the provider returned to the caller. + var logger = sp.GetService()?.CreateLogger(typeof(ServiceProviderFactory).FullName!); + if (logger != null) { - if (!openGeneric.IsKeyedService) - _logOpenGenericWarning.Invoke(logger, openGeneric.ServiceKey, openGeneric.ServiceType, openGeneric.ImplementationType!, null); - else - _logOpenGenericWarning.Invoke(logger, openGeneric.ServiceKey, openGeneric.ServiceType, openGeneric.KeyedImplementationType!, null); + foreach (var openGeneric in openGenerics) + { + var implementationType = openGeneric.IsKeyedService + ? openGeneric.KeyedImplementationType + : openGeneric.ImplementationType; + _logOpenGenericWarning.Invoke(logger, openGeneric.ServiceKey, openGeneric.ServiceType, implementationType!, null); + } } } + catch (Exception) + { + // Optional warning delivery must not prevent ownership from reaching + // the caller, even if logger activation or logging fails. + } } return sp; }