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

Azure Active Directory(AAD): Fixes stuck requests when background refresh fails to refresh token #2697

Merged
merged 11 commits into from
Sep 9, 2021

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Sep 2, 2021

Pull Request Template

Description

  1. This bug can cause requests to get stuck because the semaphore was not getting released in the scenario where multiple requests are waiting for a new token. This only occurs in scenario where the background refresh has failed to get a new token.
  2. This optimizes the scenario where multiple concurrent requests are waiting on the token. It will now return the original task that is getting the token. This prevents all the requests waiting in serial to get the failure. All the requests will return the same exception.

The existing test for this scenario use .Wait which blocked the threads which also blocked all the other task to simulate concurrent requests. The tasks now use a Task.Run to prevent them from getting blocked again and wait logic was converted to an async/await.

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Closing issues

To automatically close an issue: closes #IssueNumber

ealsur
ealsur previously approved these changes Sep 7, 2021
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank for the refactoring. I like the current synchronization mechanism.

@j82w j82w merged commit 660ec97 into master Sep 9, 2021
@j82w j82w deleted the users/jawilley/aad/FixStuckSempahore branch September 9, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure Active Directory bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants