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.34k stars 9.98k forks source link

DefaultHttpResponse throws null ref when trying to access StatusCode (inside the AspNetCore library) #52760

Open aGarik opened 10 months ago

aGarik commented 10 months ago

Is there an existing issue for this?

Describe the bug

We are experiencing a very similar issue to https://github.com/dotnet/aspnetcore/issues/25454.

During the health check liveness probe, we sometimes encounter an "Object reference not set to an instance of an object." exception. However, it's important to note that this exception is not occurring within our code (as it did in the related ticket) but rather inside the AspNetCore library.

Expected Behavior

No response

Steps To Reproduce

Unfortunately, I was unable to find a way to reproduce the issue locally, but it does occur intermittently in our production environment, although not very frequently

Exceptions (if any)

NullReferenceException: Object reference not set to an instance of an object.

at Microsoft.AspNetCore.Http.DefaultHttpResponse.get_StatusCode() at App.Metrics.AspNetCore.Tracking.Middleware.ErrorRequestMeterMiddleware.Invoke(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.ActiveRequestCounterEndpointMiddleware.Invoke(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.ApdexMiddleware.MeasureTransaction(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.PerRequestTimerMiddleware.TimeTransaction(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.RequestTimerMiddleware.TimeTransaction(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.ErrorRequestMeterMiddleware.Invoke(HttpContext context) at App.Metrics.AspNetCore.Tracking.Middleware.ActiveRequestCounterEndpointMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)

.NET Version

7.0.5

Anything else?

Host: Version: 7.0.5 Architecture: x64 Commit: 8042d61b17

.NET SDKs installed: No SDKs were found.

.NET runtimes installed: Microsoft.AspNetCore.App 7.0.5 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.NETCore.App 7.0.5 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

Tratcher commented 10 months ago

App.Metrics.AspNetCore.Tracking.Middleware is your code, correct? This class of issues is almost always caused by incorrect mutli-threading access to the HttpContext. I haven't seen it happen with the StatusCode field before though.

Does it repro with .NET 8?

aGarik commented 10 months ago

App.Metrics.AspNetCore.Tracking.Middleware is your code, correct? This class of issues is almost always caused by incorrect mutli-threading access to the HttpContext. I haven't seen it happen with the StatusCode field before though.

No, it is not our code, it is this library - https://www.app-metrics.io/web-monitoring/aspnet-core/tracking-middleware/

The code of the last middleware in stack trace: https://github.com/AppMetrics/AppMetrics/blob/31e5888d97d6ea799d96598c35411da95b95869b/src/AspNetCore/src/App.Metrics.AspNetCore.Tracking/Middleware/ErrorRequestMeterMiddleware.cs#L50

              _logger.MiddlewareExecuting<ErrorRequestMeterMiddleware>(); 

               await _next(context);

               var routeTemplate = context.GetMetricsCurrentRouteName();

                if (!context.Response.IsSuccessfulResponse() && _ignoredHttpStatusCodes.All(i => i != context.Response.StatusCode))
                {
                    _metrics.RecordHttpRequestError(routeTemplate, context.Response.StatusCode, _bucketTimerOptions);
                }

Does it repro with .NET 8?

Not tried on .net8, current is .net 7

okonaraddi-msft commented 9 months ago

Same issue being observed on .NET 6

System.NullReferenceException: Object reference not set to an instance of an object.
at Microsoft.AspNetCore.Http.DefaultHttpResponse.get_StatusCode()

Since this a public forum and our app is internal to Microsoft, the above is a minimal snippet of the stack trace (I can be reached for more details by Microsoft employees via Teams). Some context: we have middleware that's attempting to read an IHttpContextAccessor's HttpContext's HttpResponse. We're doing a null check on HttpResponse immediately prior to attempting to access the StatusCode. We're seeing a null reference exception when attempting to access .StatusCode, access .Headers, or call .OnCompleted.

@Tratcher , as far as I can tell, we don't have explicit application code for multi-threaded access to HttpContext.

Tratcher commented 9 months ago

@okonaraddi-msft Where is IHttpContextAccessor being used? That can often lead to accidental async access, such as from log providers.

okonaraddi-msft commented 9 months ago

That looks like what it is, accidental async access because we capture IHttpContextAccessor and use it in a delegating handler!

We register a singleton IHttpContextAccessor and then register many singletons (let's call these Vegetables) that accept IHttpContextAccessor in their constructor. The Vegetables are DI'ed into middleware and a delegating handler used by several http clients.