Register endpoints as Singleton instead

[dotnetjunkie]

The AddEndpoints extension method registers all IEndpoint implementation as Scoped. Registering them as scoped, however, is dangerous, because it could easily lead to undetected Captive Dependencies, memory leaks, concurrency bugs, and other hard-to-spot issues. I will explain why this is.

The home page shows the following example endpoint:

public class GetWithParamEndpoint : IEndpoint<string, string>
{
    public void AddRoute(IEndpointRouteBuilder app)
    {
        app.MapGet("/Todo/2/{param1}", (string param1) =>
            HandleAsync(param1));
    }

    public Task<string> HandleAsync(string request)
    {
        return Task.FromResult($"Hello World! 2 {request}");
    }
}

When MapEndpoints is called at startup, this endpoint will be resolved from a newly created scope and its AddRoute method will be called.

At that point, GetWithParamEndpoint will register its HandleAsync method to the MapGet method. This effectively keeps the GetWithParamEndpoint instance alive for the duration of the application. This has the following consequences:

• The HandleAsync method of the same GetWithParamEndpoint instance can be called in parallel by multiple requests, as ASP.NET Core does not implement any synchronization on this
• If GetWithParamEndpoint gets any dependencies injected into its constructor, those dependencies might, therefore, be called in parallel, which will typically be a problem for Scoped dependencies.
• Those injected dependencies with will stay referenced for the duration of the application. This is problematic for dependencies such as DbContext implementations, as they cache data and become stale over time.

It seems to me that you are partly aware of the issues that this model might cause, because I noticed that MapEndpoints doesn't dispose its created scope. If MapEndpoints would dispose of the scope from which GetWithParamEndpoint is resolved, it will quickly cause trouble when either GetWithParamEndpoint or one of its (direct or indirect) Transient or Scoped dependencies implements IDisposable - DbContext being the most obvious one. You likely noticed this issue, which probably lead to removal of the call to Dispose from MapEndpoints. But I'm afraid that removing Dispose on the IServiceScope only fixed the symptoms, not the underlying problem.

These problems popup when GetWithParamEndpoint gets injected dependencies. Any endpoint with no dependencies doesn't exhibit these issues. It still might have concurrency issues in case it contains mutable state, but I'd say that this is a less likely scenario.

Probably the simplest way around this issue to let AddEndpoints register all endpoints as Singleton instead of Scoped. This doesn't change the way the application is working - the endpoints were effectively Singletons anyway. But it allows MS.DI and the framework to validate the endpoints' object graphs. MS.DI disallows injecting scoped dependencies into Singletons (and for good reason). This means that whenever a user tries to inject a DbContext into their endpoint class, the framework will prevent this - as it should, as it will absolutely lead to bugs down the line.

In this case you should also resolve the endpoints from the root container, rather than creating a service scope, as what's happening in MapEndpoints. This service scope goes out of scope at the end of MapEndpoints while it is never disposed of, not even when the application ends. This means that any disposable Transient that is part of the endpoints object graph, will also never get disposed of when the application ends. This can be easily solved by resolving from the root container as those disposable transients will be tracked by the root container and the framework will ensure the root container is disposed of when the application ends.

This is how to resolve from the root scope:

public static void MapEndpoints(this WebApplication builder)
{
    // builder.Services is the root scope.
    var endpoints = builder.Services.GetServices<IEndpoint>();

    foreach (var endpoint in endpoints)
    {
        endpoint.AddRoute(builder);
    }
}

Of course, making endpoints Singleton is not your only option, but probably the simplest - it has the least impact on usability.

Another option is to leave the current behavior in tact and instead document explicitly that endpoints should not have scoped dependencies, or document that endpoints should have no dependencies at all. Problem, of course, is that when injecting seems to work, developers will do it anyway. Developers are notoriously bad in reading the fine manual.

The issue resolves around the definition of an abstraction that has its AddRoute method called at startup, while its HandleAsync method is called on a per-request basis. A last option to consider, therefore, is to radically change the design of this library. You could try to design this api such that endpoints are resolved per request and their HandleAsync method is called on that resolved instance. This might, however, be a complete redesign and I certainly understand if that's not a change you're willing to make.

I hope this all makes sense.