Skip to content

Commit

Permalink
Add BuildServiceProvider analyzer rule (#192)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
  • Loading branch information
rannes and joeldickson authored Oct 17, 2024
1 parent 44c34b4 commit 57aeae5
Show file tree
Hide file tree
Showing 5 changed files with 321 additions and 1 deletion.
121 changes: 121 additions & 0 deletions doc/AG0043.md
Original file line number Diff line number Diff line change
@@ -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<IMyService, MyService>();
var serviceProvider = services.BuildServiceProvider(); // Noncompliant
var myService = serviceProvider.GetService<IMyService>();
}
```

#### Minimal API
```csharp
var builder = WebApplication.CreateBuilder(args);

builder.Services.AddTransient<IMyService, MyService>();

var app = builder.Build();

app.MapGet("/", () =>
{
var serviceProvider = app.Services.BuildServiceProvider(); // Noncompliant
var myService = serviceProvider.GetService<IMyService>();
return myService.GetMessage();
});

app.Run();
```

### Compliant Solutions

#### Traditional ASP.NET Core
```csharp
public class Startup
{
public void ConfigureServices(IServiceCollection services)
{
services.AddTransient<IMyService, MyService>();
// 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<IMyService, MyService>();

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<T>()` to get scoped services
2. For configuration validation:
```csharp
public void ConfigureServices(IServiceCollection services)
{
services.AddTransient<IMyService, MyService>();
// 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)
96 changes: 96 additions & 0 deletions src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs
Original file line number Diff line number Diff line change
@@ -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<object>();
}
}"
};

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<object>()
.BuildServiceProvider();
}
}"
};

await VerifyDiagnosticsAsync(code, new DiagnosticLocation(10, 34));
}
}
94 changes: 94 additions & 0 deletions src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs
Original file line number Diff line number Diff line change
@@ -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,

Check warning on line 30 in src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs

View workflow job for this annotation

GitHub Actions / Build Package

Rule 'AG0043' is not part of any analyzer release

Check warning on line 30 in src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs

View workflow job for this annotation

GitHub Actions / Build Package

Rule 'AG0043' is not part of any analyzer release
Title,

Check warning on line 31 in src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs

View workflow job for this annotation

GitHub Actions / Build Package

The diagnostic title should not contain a period, nor any line return character, nor any leading or trailing whitespaces
MessageFormat,
AnalyzerCategory.CustomQualityRules,
DiagnosticSeverity.Error,
AnalyzerConstants.EnabledByDefault,
Description,
"https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0043.md",
WellKnownDiagnosticTags.EditAndContinue);

public override ImmutableArray<DiagnosticDescriptor> 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<IMethodSymbol>()
.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);
}
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -260,4 +260,7 @@ One exception is logging, where it can be useful to see the exact DC / cluster /
<data name="AG0041Title" xml:space="preserve">
<value>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</value>
</data>
</root>
<data name="AG0043Title" xml:space="preserve">
<value>BuildServiceProvider detected in request processing code. This may cause memory leaks. Remove the BuildServiceProvider() call and let the framework manage the service provider lifecycle.</value>
</data>
</root>

0 comments on commit 57aeae5

Please sign in to comment.