DuendeSoftware / Support

Support for Duende Software products
21 stars 0 forks source link

UnhandledExceptionLoggingFilter should handle OperationCanceledException instead of TaskCanceledException #1377

Closed danchristensen2000 closed 1 month ago

danchristensen2000 commented 2 months ago

Which version of Duende IdentityServer are you using? 7.0.6

Which version of .NET are you using? .net 8

Describe the bug OperationCanceledExceptions from EF core are logged as Critical errors.

To Reproduce I don't know exactly how to reproduce this, but I can include the stack trace for our logs.

Expected behavior I would expect the UnhandledExceptionLoggingFilter to prevent logging of an OperationCanceledException.

Log output/exception with stacktrace System.OperationCanceledException: The operation was canceled. at System.Threading.CancellationToken.ThrowOperationCanceledException() at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteImplementationAsync[TState,TResult](Func4 operation, Func4 verifySucceeded, TState state, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func4 operation, Func4 verifySucceeded, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable1.AsyncEnumerator.MoveNextAsync() at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable1 source, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable1 source, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToArrayAsync[TSource](IQueryable1 source, CancellationToken cancellationToken) at Duende.IdentityServer.EntityFramework.Stores.PersistedGrantStore.StoreAsync(PersistedGrant token) in //src/EntityFramework.Storage/Stores/PersistedGrantStore.cs:line 59 at Duende.IdentityServer.Stores.DefaultGrantStore1.StoreItemByHashedKeyAsync(String hashedKey, T item, String clientId, String subjectId, String sessionId, String description, DateTime created, Nullable1 expiration, Nullable`1 consumedTime) in //src/IdentityServer/Stores/Default/DefaultGrantStore.cs:line 231 at Duende.IdentityServer.Stores.DefaultGrantStore`1.CreateItemAsync(T item, String clientId, String subjectId, String sessionId, String description, DateTime created, Int32 lifetime) in //src/IdentityServer/Stores/Default/DefaultGrantStore.cs:line 177 at Duende.IdentityServer.ResponseHandling.AuthorizeResponseGenerator.CreateCodeFlowResponseAsync(ValidatedAuthorizeRequest request) in //src/IdentityServer/ResponseHandling/Default/AuthorizeResponseGenerator.cs:line 143 at Duende.IdentityServer.ResponseHandling.AuthorizeResponseGenerator.CreateResponseAsync(ValidatedAuthorizeRequest request) in //src/IdentityServer/ResponseHandling/Default/AuthorizeResponseGenerator.cs:line 100 at Duende.IdentityServer.Endpoints.AuthorizeEndpointBase.ProcessAuthorizeRequestAsync(NameValueCollection parameters, ClaimsPrincipal user, Boolean checkConsentResponse) in //src/IdentityServer/Endpoints/AuthorizeEndpointBase.cs:line 148 at Duende.IdentityServer.Endpoints.AuthorizeEndpointBase.ProcessAuthorizeRequestAsync(NameValueCollection parameters, ClaimsPrincipal user, Boolean checkConsentResponse) in //src/IdentityServer/Endpoints/AuthorizeEndpointBase.cs:line 160 at Duende.IdentityServer.Endpoints.AuthorizeEndpoint.ProcessAsync(HttpContext context) in //src/IdentityServer/Endpoints/AuthorizeEndpoint.cs:line 64 at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IdentityServerOptions options, IEndpointRouter router, IUserSession userSession, IEventService events, IIssuerNameService issuerNameService, ISessionCoordinationService sessionCoordinationService) in //src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 106 at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IdentityServerOptions options, IEndpointRouter router, IUserSession userSession, IEventService events, IIssuerNameService issuerNameService, ISessionCoordinationService sessionCoordinationService) in //src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 128 at Duende.IdentityServer.Hosting.MutualTlsEndpointMiddleware.Invoke(HttpContext context, IAuthenticationSchemeProvider schemes) in //src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs:line 95 at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) at Duende.IdentityServer.Hosting.DynamicProviders.DynamicSchemeAuthenticationMiddleware.Invoke(HttpContext context) in //src/IdentityServer/Hosting/DynamicProviders/DynamicSchemes/DynamicSchemeAuthenticationMiddleware.cs:line 51 at Duende.IdentityServer.Hosting.BaseUrlMiddleware.Invoke(HttpContext context) in /_/src/IdentityServer/Hosting/BaseUrlMiddleware.cs:line 27 at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionMiddleware.InvokeCore(HttpContext context) at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context) at Nbs.Framework.Startup.Middleware.HttpRequestLoggingPropertiesMiddleware.Invoke(HttpContext context) at Nbs.Framework.Startup.Middleware.ResponseHandlerMiddleware.Invoke(HttpContext context)

Additional context The default UnhandledExceptionLoggingFilter is handling TaskCanceledException, which is derived from OperationCancelledException. Please read Stephen Cleary's blog on this topic, where he recommends handling OperationCancelledException instead of TaskCanceledException because some APIs just raise OperationCanceledException, even if they deal with cancelled tasks.

https://blog.stephencleary.com/2022/03/cancellation-3-detecting-cancellation.html#:~:text=And%20since%20TaskCanceledException%20derives%20from,Catch%20OperationCanceledException%20instead.

RolandGuijt commented 1 month ago

Thanks for reporting this. I've created an issue in the IdentityServer repository for this. We're going to investigate further and possibly make code changes. Please track the new issue for updates. I'm closing this one.