dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.45k stars 10.03k forks source link

Sometimes service marked with [FromServices] attribute isn't injected into controller's action #48671

Open Markeli opened 1 year ago

Markeli commented 1 year ago

Is there an existing issue for this?

Describe the bug

We have controller's action like this:

public async Task<ActionResult<APIResponse>> Upload(
    [FromQuery] string sid,
    [FromQuery] string uniqueKey,
    [FromQuery] string fileName,
    [FromServices] ISftpClientFactory sftpClientFactory,
    [FromServices] ITempFileStreamFactory tempFileStreamFactory)
{
    if (sftpClientFactory == null) throw new ArgumentNullException(nameof(sftpClientFactory));
    if (tempFileStreamFactory == null) throw new ArgumentNullException(nameof(tempFileStreamFactory));

    // some logic goes here
}

We injects ISftpClientFactory and ITempFileStreamFactory instances as dependencies. They marked with [FromServices]. All services are registered.

99.99% of all request processed correctly. But sometimes we get null instead of this services. And null checks throws an exception:

System.ArgumentNullException: Value cannot be null. (Parameter 'sftpClientFactory')

We doesn't change request's pipeline dynamically. Here is our configured pipeline:

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
    if (app == null) throw new ArgumentNullException(nameof(app));
    if (env == null) throw new ArgumentNullException(nameof(env));

    app.UseRouting();
    app.UseMiddleware<ApiGlobalExceptionHandleMiddleware>();

    app.UseReverseProxy(Configuration.ReverseProxy);

    app.UseCors("CorsPolicy");

    app.UseMiddleware<PerformanceMeasureMiddleware>();

    if (Configuration.LogRequestContent)
        app.UseMiddleware<RequestLoggingMiddleware>();

    var localizationOptions = app.ApplicationServices.GetService<IOptions<RequestLocalizationOptions>>();
    app.UseRequestLocalization(localizationOptions.Value);

    app.UseEndpoints(endpoints =>
    {
        endpoints.MapControllerRoute(
            name: "DefaultApi",
            pattern: "{version}/{controller}/{action}",
            constraints: new { version = "v4|v5|v6" });
    });
}

All of this middleware doesn't change pipeline's flow.

Here is example of registration of those services:

 public void ConfigureServices(IServiceCollection services)
 {
        services.TryAddSingleton<ISftpClientFactory, SftpClientFactory>();
        services.TryAddSingleton<ITempFileStreamFactory, TempFileStreamFactory>();

        // other services registration...
}

Expected Behaviour

Dependencies are always not null

Steps To Reproduce

No specific steps, the error occurs randomly.

Exceptions (if any)

System.ArgumentNullException: Value cannot be null. (Parameter 'sftpClientFactory')
at SIISLtd.SSNG.WebAPI.Controllers.FileController.Upload(String sid, String uniqueKey, String fileName, ISftpClientFactory sftpClientFactory, ITempFileStreamFactory tempFileStreamFactory) in /home/xpg934/ssng/src/Web/SIISLtd.SSNG.WebAPI/Controllers/FileController.cs:line 110
at lambda_method2228(Closure , Object )
at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Logged|12_1(ControllerActionInvoker invoker)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Logged|17_1(ResourceInvoker invoker)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Logged|17_1(ResourceInvoker invoker)
at Microsoft.AspNetCore.Routing.EndpointMiddleware.g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
at SIISLtd.SSNG.WebTools.Middleware.RequestLoggingMiddleware.InvokeAsync(HttpContext context) in /home/xpg934/ssng/src/Web/SIISLtd.SSNG.WebTools/Middleware/RequestLoggingMiddleware.cs:line 70
at SIISLtd.SSNG.WebTools.Middleware.PerformanceMeasureMiddleware.InvokeAsync(HttpContext context) in /home/xpg934/ssng/src/Web/SIISLtd.SSNG.WebTools/Middleware/PerformanceMeasureMiddleware.cs:line 48
at SIISLtd.SSNG.WebAPI.Middleware.ApiGlobalExceptionHandleMiddleware.InvokeAsync(HttpContext httpContext) in /home/xpg934/ssng/src/Web/SIISLtd.SSNG.WebAPI/Middleware/ApiGlobalExceptionHandleMiddleware.cs:line 32

.NET Version

6.0.0

Anything else?

Host (useful for support): Version: 6.0.0 Commit: 4822e3c3aa

.NET SDKs installed: No SDKs were found.

.NET runtimes installed: Microsoft.AspNetCore.All 2.2.8 [/usr/share/dotnet/shared/Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.2.8 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.12 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.2.8 [/usr/share/dotnet/shared/Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.12 [/usr/share/dotnet/shared/Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET runtimes or SDKs: https://aka.ms/dotnet-download

martincostello commented 1 year ago

Where in your startup code do you actually register those services? Your issue description doesn't include any parts of your configuration for IServiceCollection that registers ISftpClientFactory or ITempFileStreamFactory.

ghost commented 1 year ago

Hi @Markeli. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

Markeli commented 1 year ago

@martincostello, updated issue description with services registration example.

We don't decorate or override this services after registration. They are definitely registered. They are injected into controller's action in 99.99% of requests.

captainsafia commented 11 months ago

@Markeli I've been trying to see if I can reproduce this locally with a minimal repo containing a web app configured with two singleton services in the DI container and a controller consuming them via the attributes and 1000 requests executed sequentially but I couldn't repro this. Some questions:

If you're able to get a minimal repro going, that would be helpful.

yunusefendi52 commented 10 months ago

This also affect in our web API.

Note that it's only affect 1-2% of 10K RPS, which means it's still a lot if we see in our monitor.

Our workaround is to use HttpContext and get service from there

captainsafia commented 10 months ago

@yunusefendi52 Thanks for sharing this info! Does this always happen in the same controller?

If you're able to share any redacted logs from the repros that would be helpful.

yunusefendi52 commented 10 months ago

@captainsafia Yes this always happen in the same controller with [FromServices]

I don't have the logs anymore; it's months ago and our logs retention is only around 2 months

Markeli commented 9 months ago

@captainsafia, unfortunately I can't provide a minimal repo. We've tried to create a minimal repo, make a workload, but it works fine. But in a production bug is reproduced sometimes.

There are answers for your questions:

We use NLog as out log provider. Here is out nuget packages related to logging:

Markeli commented 9 months ago

@yunusefendi52, can you please share info about your app? Do you use NLog? And what dotnet version you use to host your app?

Markeli commented 9 months ago

@captainsafia, what kind of logs can help you to understand the reason? We can't share all our logs, but we can save for you useful logs from Microsoft classes or something else.

yunusefendi52 commented 9 months ago

@Markeli we use Serilog and dotnet 6

Markeli commented 7 months ago

We have found steps to reproduce bug.

There are at least 2 case when we've gotten this error.

Case 1: Uploading big files

Steps to reproduce:

  1. Create controller to file upload
  2. Set some limit for request body
  3. Upload file bigger than limit

Expected behavior

Dependencies are injected.

Actual behavior

Dependencies not injected.

Demo

There are demo for this case. Bug is reproduced both at .NET 6 and .NET 8.

[Uploading ExceptionDemonstration.Net6.zip…]() ExceptionDemonstration.Net8.zip

Case 2: cancelling HTTP request

Bug occurs when request was cancelled by user before model binding completion.

Steps to reproduce:

  1. Create simple webapp with action receiving some model
  2. Add custom middleware with Task.Delay(TimeSpan.FromSeconds(15);
  3. Start http request in browser and stop page loading while custom middleware is waiting delay.

In both cases ASP.NET Core model binders and services providers handles exception/request cancellation, doesn't complete their logic and continues request's flow as usual. But due to request cancelation/exception request going to controllers in some kind of broken state.

Workaround

It's possible to create custom action filter that checks request cancellation on tries to check existence of request size limit exception and throws exception that should be correctly handle by error handling middleware.

Here is out workaround sample:

public class BinderFailureActionFilter : ActionFilterAttribute
{
    public override void OnActionExecuting(ActionExecutingContext context)
    {
        // chec request cancellation
        context.HttpContext.RequestAborted.ThrowIfCancellationRequested();

        // try to check thath all DI provided services are binded
        var failedServiceParameter = context.ActionDescriptor.Parameters
            .Where(x => x.BindingInfo?.BindingSource == BindingSource.Services)
            .Where(x => !context.ActionArguments.ContainsKey(x.Name))
            .FirstOrDefault();

        // if some of service is missing, let's throw an exception
        if (failedServiceParameter != null)
            throw new BindingFailureException(context.ActionDescriptor.DisplayName, failedServiceParameter.Name);
    }
}