From 57aeae55a1b55a48fe25dddb6a6b13a3f454f743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rannes=20P=C3=A4rn?= Date: Thu, 17 Oct 2024 16:06:23 +0700 Subject: [PATCH] Add BuildServiceProvider analyzer rule (#192) * feat: add BuildServiceProvider analyzer rule * Update AG0041LogTemplateAnalyzer.cs (#193) * Update AG0041LogTemplateAnalyzer.cs * Create AG0041.md * feat: add BuildServiceProvider analyzer rule * docs: update docs for the rule. * chore: remove comments * Add examples with minimal API which is more common * Update error message with more explanation --------- Co-authored-by: Joel Dickson <9032274+joeldickson@users.noreply.github.com> --- doc/AG0043.md | 121 ++++++++++++++++++ .../AgodaCustom/AG0043UnitTests.cs | 96 ++++++++++++++ .../AG0043NoBuildServiceProvider.cs | 94 ++++++++++++++ .../CustomRulesResources.Designer.cs | 6 + .../AgodaCustom/CustomRulesResources.resx | 5 +- 5 files changed, 321 insertions(+), 1 deletion(-) create mode 100644 doc/AG0043.md create mode 100644 src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs create mode 100644 src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs diff --git a/doc/AG0043.md b/doc/AG0043.md new file mode 100644 index 0000000..0e1861e --- /dev/null +++ b/doc/AG0043.md @@ -0,0 +1,121 @@ +# AG0043: BuildServiceProvider should not be used in production code + +## Problem Description +Using `BuildServiceProvider()` in production code can lead to memory leaks and other issues because it creates a new container. This is especially problematic when called repeatedly, such as in request processing scenarios. + +## Rule Details +This rule raises an issue when `BuildServiceProvider()` is called on `IServiceCollection` instances. + +### Noncompliant Code Examples + +#### Traditional ASP.NET Core +```csharp +public void ConfigureServices(IServiceCollection services) +{ + services.AddTransient(); + var serviceProvider = services.BuildServiceProvider(); // Noncompliant + var myService = serviceProvider.GetService(); +} +``` + +#### Minimal API +```csharp +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddTransient(); + +var app = builder.Build(); + +app.MapGet("/", () => +{ + var serviceProvider = app.Services.BuildServiceProvider(); // Noncompliant + var myService = serviceProvider.GetService(); + return myService.GetMessage(); +}); + +app.Run(); +``` + +### Compliant Solutions + +#### Traditional ASP.NET Core +```csharp +public class Startup +{ + public void ConfigureServices(IServiceCollection services) + { + services.AddTransient(); + // Let ASP.NET Core build the service provider + } + public void Configure(IApplicationBuilder app, IMyService myService) + { + // Services are injected by the framework + } +} +``` + +#### Minimal API +```csharp +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddTransient(); + +var app = builder.Build(); + +app.MapGet("/", (IMyService myService) => myService.GetMessage()); + +app.Run(); + +// Service interfaces and implementations +public interface IMyService +{ + string GetMessage(); +} + +public class MyService : IMyService +{ + public string GetMessage() => "Hello from MyService!"; +} +``` + +## Why is this an Issue? +1. **Memory Leaks**: Each call to `BuildServiceProvider()` creates a new dependency injection container, which holds references to all registered services. If called repeatedly (e.g., during request processing), this leads to memory leaks. +2. **Performance Impact**: Creating new service providers is computationally expensive and can impact application performance. +3. **Singleton Duplication**: Multiple service providers can result in multiple instances of services that should be singletons. + +## Exceptions +`BuildServiceProvider()` may be acceptable in the following scenarios: +- Unit tests where a full DI container is needed +- Development-time configuration validation +- Tools and utilities that run outside the normal application lifecycle + +## How to Fix It +1. In ASP.NET Core applications: + - Let the framework handle service provider creation + - Use constructor injection to obtain dependencies + - Use `IServiceScope` for creating scoped service providers when needed + - `HttpContext` and other services have methods like `RequestServices.GetService()` to get scoped services +2. For configuration validation: + ```csharp + public void ConfigureServices(IServiceCollection services) + { + services.AddTransient(); + // Only in development + if (Environment.IsDevelopment()) + { + // Validate service registration + var serviceProvider = services.BuildServiceProvider(validateScopes: true); + serviceProvider.Dispose(); + } + } + ``` + +## Benefits +- Prevents memory leaks +- Improves application performance +- Ensures proper singleton behavior +- Maintains proper service scoping + +## References +- [ASP.NET Core Dependency Injection Best Practices](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection#service-lifetimes) +- [Dependency injection in ASP.NET Core](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection) diff --git a/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs new file mode 100644 index 0000000..3b80601 --- /dev/null +++ b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs @@ -0,0 +1,96 @@ +using System.Threading.Tasks; +using Agoda.Analyzers.AgodaCustom; +using Agoda.Analyzers.Test.Helpers; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using Microsoft.Extensions.DependencyInjection; + +namespace Agoda.Analyzers.Test.AgodaCustom; + +[TestFixture] +class AG0043UnitTests : DiagnosticVerifier +{ + protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0043NoBuildServiceProvider(); + + protected override string DiagnosticId => AG0043NoBuildServiceProvider.DIAGNOSTIC_ID; + + [Test] + public async Task BuildServiceProvider_Used_RaisesDiagnostic() + { + var services = new ServiceCollection(); + services.BuildServiceProvider(); + + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var services = new ServiceCollection(); + var provider = services.BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 53)); + } + + [Test] + public async Task OtherMethod_NoDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] {typeof(ServiceCollection).Assembly}, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + class TestClass + { + public void ConfigureServices() + { + var services = new ServiceCollection(); + services.AddSingleton(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task BuildServiceProvider_WhenChaining_RaiseDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var provider = new ServiceCollection() + .AddSingleton() + .BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(10, 34)); + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs new file mode 100644 index 0000000..8653f89 --- /dev/null +++ b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs @@ -0,0 +1,94 @@ +using System.Collections.Immutable; +using System.Linq; +using Agoda.Analyzers.Helpers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Agoda.Analyzers.AgodaCustom +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class AG0043NoBuildServiceProvider : DiagnosticAnalyzer + { + public const string DIAGNOSTIC_ID = "AG0043"; + + private static readonly LocalizableString Title = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString MessageFormat = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString Description + = DescriptionContentLoader.GetAnalyzerDescription(nameof(AG0043NoBuildServiceProvider)); + + private static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( + DIAGNOSTIC_ID, + Title, + MessageFormat, + AnalyzerCategory.CustomQualityRules, + DiagnosticSeverity.Error, + AnalyzerConstants.EnabledByDefault, + Description, + "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0043.md", + WellKnownDiagnosticTags.EditAndContinue); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeNode(SyntaxNodeAnalysisContext context) + { + var invocationExpr = (InvocationExpressionSyntax)context.Node; + + if (!(invocationExpr.Expression is MemberAccessExpressionSyntax memberAccessExpr)) + return; + + if (memberAccessExpr.Name.Identifier.ValueText != "BuildServiceProvider") + return; + + var methodSymbol = context.SemanticModel.GetSymbolInfo(invocationExpr).Symbol as IMethodSymbol; + if (methodSymbol == null) + return; + + if (!methodSymbol.ContainingNamespace.ToDisplayString() + .StartsWith("Microsoft.Extensions.DependencyInjection")) + return; + + var containingType = methodSymbol.ContainingType; + if (containingType == null) + return; + + var isServiceCollectionExtension = containingType + .GetTypeMembers() + .SelectMany(t => t.GetMembers()) + .OfType() + .Any(m => m.Parameters.Length > 0 && + m.Parameters[0].Type.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + var expressionType = context.SemanticModel.GetTypeInfo(memberAccessExpr.Expression).Type; + var isExpressionServiceCollection = expressionType != null && + (expressionType.AllInterfaces.Any(i => + i.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection") || + expressionType.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + if (isServiceCollectionExtension || isExpressionServiceCollection) + { + var diagnostic = Diagnostic.Create(Descriptor, memberAccessExpr.Name.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs index 905e367..b7dca7a 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs @@ -323,5 +323,11 @@ public static string AG0041Title { return ResourceManager.GetString("AG0041Title", resourceCulture); } } + + public static string AG0043Title { + get { + return ResourceManager.GetString("AG0043Title", resourceCulture); + } + } } } diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index 1f99dcc..5771a6a 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -260,4 +260,7 @@ One exception is logging, where it can be useful to see the exact DC / cluster / You are using either an interpolated string or string concatenation in your logs, change these to the message template format to preserve structure in your logs - \ No newline at end of file + + BuildServiceProvider detected in request processing code. This may cause memory leaks. Remove the BuildServiceProvider() call and let the framework manage the service provider lifecycle. + +