Skip to content

Conversation

@Avery-Dunn
Copy link
Contributor

@Avery-Dunn Avery-Dunn commented May 30, 2025

This PR refactors HTTP retry logic by splitting it into classes that implement a new interface, similar to what was done in MSAL Node and .NET:
AzureAD/microsoft-authentication-library-for-js#7614
AzureAD/microsoft-authentication-library-for-dotnet#5231

The following changes were made:

  • A new IRetryPolicy interface, defining methods to determine if a certain HTTP response should be retried, the number of retries to perform, and how long to wait between retries
  • Three new classes that implement IRetryPolicy:
    • IMDSRetryPolicy, for behavior specific to IMDS
    • ManagedIdentityRetryPolicy, for all other managed identity scenarios
    • DefaultRetryPolicy, for all other scenarios
  • Minor changes to internal APIs and method calls to use the new classes

For testing:

  • Much of the retry behavior remains the same and is already covered by unit tests, such as those found in RequestThrottlingTest and ManagedIdentityTests.managedIdentityTest_Retry()
  • The exception is the different retry behaviors in IMDSRetryPolicy based on status codes, which is now covered by the new unit test in ManagedIdentityTests
  • In addition, each retry policy class was given package-private methods to adjust the delays between retries, to significantly speed up the unit tests

Most of the "files changed" list in this PR are due to changes in two other PRs:
#960
#964

@Avery-Dunn Avery-Dunn requested a review from a team as a code owner May 30, 2025 00:41
@@ -0,0 +1,25 @@
package com.microsoft.aad.msal4j;
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: All this HTTP and Retry files should go into a folder of their own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be good to group similar files together, but currently all of the source files are just in one big folder. I don't know why it was originally designed that way (the people who would did it are no longer here), and I've always felt it was a bad practice.

I've created an issue to track a refactor of the directory structure, at the very least there's quick value in properly organizing the non-public classes: #965

Add API to disable internal retry behavior
@Avery-Dunn Avery-Dunn merged commit ae56785 into dev Jun 5, 2025
5 checks passed
@Avery-Dunn Avery-Dunn deleted the avdunn/retry-behavior branch September 15, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants