Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread starvation due to sync over async code in IdentityModel.Tokens.Validators #2556

Open
mirjana1211 opened this issue Apr 10, 2024 · 1 comment
Assignees
Labels
Bug Product is not functioning as expected P1 More important, prioritize highly sync-over-async

Comments

@mirjana1211
Copy link

Microsoft.Identity.Web Library

Microsoft.Identity.Web

Microsoft.Identity.Web version

2.17.2

Web app

Not Applicable

Web API

Protected web APIs (validating tokens)

Token cache serialization

Not Applicable

Description

We have been experiencing sporadic issues with our application deployed as an Azure App Service running on Linux (.net8).
We are using Microsoft.Identity.Web library and Microsoft Entra ID as our identity provider.

From time to time, we get an alert for failed health checks for one of the instances (minimum of 6 instances running). The instance seems to be unresponsive and for a period of time and the issue always resolves itself within a few minutes. There is no relevant info in the logs or in the Diagnose and solve problems section for the app service.
We can not identify any changes in request rate or resource utilization before the outage.

We have created a support case with Microsoft and have provided multiple memory dumps for analysis. The information we got from the support engineer is that the app service failure is caused by thread starvation due to sync over async code in IdentityModel.

Reproduction steps

The outage occurs sporadically and it is not possible to reproduce it on demand.

The problem seems to be related to load. We have several instances of the same application deployed in different Azure regions and the only instance where we have observed the problem is running under a relatively high load (~500-600 req/sec during peak time).

Error message

No response

Id Web logs

No response

Relevant code snippets

The code in question, according to the dumps analysis, is in the ValidateIssuer method: 

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/src/Microsoft.IdentityModel.Tokens/Validators.cs.

Regression

No response

Expected behavior

No application outages due to blocking calls :)

@jmprieur jmprieur added Investigate We are not quite sure what the issue is. and removed Untriaged Needs attention labels Apr 11, 2024
@jennyf19 jennyf19 transferred this issue from AzureAD/microsoft-identity-web Apr 11, 2024
@jennyf19 jennyf19 added sync-over-async P1 More important, prioritize highly labels Apr 11, 2024
@victorm-hernandez victorm-hernandez self-assigned this Apr 16, 2024
victorm-hernandez pushed a commit that referenced this issue Apr 19, 2024
Exposed the ValidateAsync methods in the AadIssuerValidator class to allow
for custom async validation of the issuer.
Exposed the IssuerValidatorAsync delegate in the TokenValidationParameters
so the custom async validation can be set.
Updated the priority logic used to decide which custom validator has priority.

Related issue: #2556
@pmaytak
Copy link
Contributor

pmaytak commented Apr 25, 2024

Based on the discussions, looks like there are 3 options:

  1. Make the currently internal ValidateIssuerAsync API in IdentityModel public and use it in Identity Web instead of the sync overload.
    Pro: Quick fix.
    Con: Async delegates API design is not fully complete. This change may make changes to this API more difficult later. Adding only one async delegate for issuer right now makes API less consistent.
  2. Make IdentityModel internals visible to Identity Web and use the internal ValidateIssuerAsync in Identity Web instead of the sync overload. In IdentityModel 8, the async delegates will be finalized and made public; IdentityModel internals can be hidden for Identity Web again.
    Pro: Quick fix and doesn't expose a new public API.
    Con: InternalsVisibleTo makes library usage and versioning more complex. May cause issues if the API needs to change later.
  3. First finish the complete feature [Feature Request] All validation delegates should be async and return a validation result #2568 (make all validation delegates async, obsolete sync ones, don't use exceptions for control flow). Update Identity Web to use async validators.
    Pro: Allows for the best thought-out solution, consistent public API.
    Con: Longest time frame. The change will be in IdentityModel 8 and Identity Web 3.

Based on all the discussions, option 3 seems to be the better one. It's better to deliver a more well-planned out solution.

@pmaytak pmaytak added Bug Product is not functioning as expected and removed Investigate We are not quite sure what the issue is. labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product is not functioning as expected P1 More important, prioritize highly sync-over-async
Projects
None yet
Development

No branches or pull requests

5 participants