PR feedback

Author: Avery-Dunn

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultRetryPolicy.java

@@ -8,7 +8,7 @@
  */
 class DefaultRetryPolicy implements IRetryPolicy {
     private static final int RETRY_NUM = 1;
-    private static int RETRY_DELAY_MS = 1000;
+    private static final int RETRY_DELAY_MS = 1000;
 
     @Override
     public boolean isRetryable(IHttpResponse httpResponse) {
@@ -25,13 +25,4 @@ public int getMaxRetryCount(IHttpResponse httpResponse) {
     public int getRetryDelayMs(IHttpResponse httpResponse) {
         return RETRY_DELAY_MS;
     }
-
-    //Package-private methods to allow much quicker testing. The delay values should be treated as constants in any non-test scenario.
-    static void setRetryDelayMs(int retryDelayMs) {
-        RETRY_DELAY_MS = retryDelayMs;
-    }
-
-    static void resetToDefaults() {
-        RETRY_DELAY_MS = 1000;
-    }
 }

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java

@@ -11,9 +11,12 @@
 //IMDS uses a different try policy than other MI flows, see https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/docs/imds_retry_based_on_errors.md
 class IMDSRetryPolicy extends ManagedIdentityRetryPolicy {
     private static final int LINEAR_RETRY_NUM = 7;
-    private static int LINEAR_RETRY_DELAY_MS = 10000; // 10 seconds
+    private static final int LINEAR_RETRY_DELAY_MS = 10000; // 10 seconds
     private static final int EXPONENTIAL_RETRY_NUM = 3;
-    private static int EXPONENTIAL_RETRY_DELAY_MS = 1000; // 1 second
+    private static final int EXPONENTIAL_RETRY_DELAY_MS = 1000; // 1 second
+
+    private static int currentLinearRetryDelayMs = LINEAR_RETRY_DELAY_MS;
+    private static int exponentialLinearRetryDelayMs = EXPONENTIAL_RETRY_DELAY_MS;
 
     private int currentRetryCount;
     private int lastStatusCode;
@@ -37,27 +40,27 @@ public boolean isRetryable(IHttpResponse httpResponse) {
 
     @Override
     public int getMaxRetryCount(IHttpResponse httpResponse) {
-        return (httpResponse.statusCode() == 410) ? LINEAR_RETRY_NUM : EXPONENTIAL_RETRY_NUM;
+        return (httpResponse.statusCode() == HttpStatus.GONE.getCode()) ? LINEAR_RETRY_NUM : EXPONENTIAL_RETRY_NUM;
     }
 
     @Override
     public int getRetryDelayMs(IHttpResponse httpResponse) {
         // Use exponential backoff for non-410 status codes
-        if (lastStatusCode == 410) {
-            return LINEAR_RETRY_DELAY_MS;
+        if (lastStatusCode == HttpStatus.GONE.getCode()) {
+            return currentLinearRetryDelayMs;
         } else {
-            return (int) (Math.pow(2, currentRetryCount) * EXPONENTIAL_RETRY_DELAY_MS);
+            return (int) (Math.pow(2, currentRetryCount) * exponentialLinearRetryDelayMs);
         }
     }
 
     //Package-private methods to allow much quicker testing. The delay values should be treated as constants in any non-test scenario.
     static void setRetryDelayMs(int retryDelayMs) {
-        LINEAR_RETRY_DELAY_MS = retryDelayMs;
-        EXPONENTIAL_RETRY_DELAY_MS = retryDelayMs;
+        currentLinearRetryDelayMs = retryDelayMs;
+        exponentialLinearRetryDelayMs = retryDelayMs;
     }
 
     static void resetToDefaults() {
-        LINEAR_RETRY_DELAY_MS = 10000;
-        EXPONENTIAL_RETRY_DELAY_MS = 1000;
+        currentLinearRetryDelayMs = LINEAR_RETRY_DELAY_MS;
+        exponentialLinearRetryDelayMs = EXPONENTIAL_RETRY_DELAY_MS;
     }
 }

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityRetryPolicy.java

@@ -13,7 +13,9 @@
  */
 class ManagedIdentityRetryPolicy implements IRetryPolicy {
     private static final int RETRY_NUM = 3;
-    private static int RETRY_DELAY_MS = 1000;
+    private static final int RETRY_DELAY_MS = 1000;
+
+    private static int currentRetryDelayMs = RETRY_DELAY_MS;
 
     private static final Set<Integer> RETRYABLE_STATUS_CODES = Collections.unmodifiableSet(
             new HashSet<>(Arrays.asList(
@@ -38,15 +40,15 @@ public int getMaxRetryCount(IHttpResponse httpResponse) {
 
     @Override
     public int getRetryDelayMs(IHttpResponse httpResponse) {
-        return RETRY_DELAY_MS;
+        return currentRetryDelayMs;
     }
 
     //Package-private methods to allow much quicker testing. The delay values should be treated as constants in any non-test scenario.
     static void setRetryDelayMs(int retryDelayMs) {
-        RETRY_DELAY_MS = retryDelayMs;
+        currentRetryDelayMs = retryDelayMs;
     }
 
     static void resetToDefaults() {
-        RETRY_DELAY_MS = 1000;
+        currentRetryDelayMs = RETRY_DELAY_MS;
     }
 }

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java

@@ -600,7 +600,7 @@ void managedIdentityTest_IMDSRetry() throws Exception {
                     .build();
 
             // IMDS has different retry logic for certain status codes, such as 410
-            when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE))).thenReturn(expectedResponse(410, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
+            when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE))).thenReturn(expectedResponse(HttpStatus.GONE.getCode(), ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
 
             try {
                 acquireTokenCommon(ManagedIdentityTestConstants.RESOURCE).get();
@@ -612,7 +612,7 @@ void managedIdentityTest_IMDSRetry() throws Exception {
             }
 
             clearInvocations(httpClientMock);
-            when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE))).thenReturn(expectedResponse(500, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
+            when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE))).thenReturn(expectedResponse(HttpStatus.INTERNAL_SERVER_ERROR.getCode(), ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
 
             try {
                 acquireTokenCommon(ManagedIdentityTestConstants.RESOURCE).get();
@@ -642,8 +642,8 @@ void managedIdentityTest_RetrySucceedsAfterFailure() throws Exception {
 
             // First call returns 500, subsequent calls return 200
             when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE)))
-                    .thenReturn(expectedResponse(500, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500))
-                    .thenReturn(expectedResponse(200, getSuccessfulResponse(ManagedIdentityTestConstants.RESOURCE)));
+                    .thenReturn(expectedResponse(HttpStatus.INTERNAL_SERVER_ERROR.getCode(), ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500))
+                    .thenReturn(expectedResponse(HttpStatus.OK.getCode(), getSuccessfulResponse(ManagedIdentityTestConstants.RESOURCE)));
 
             IAuthenticationResult result = acquireTokenCommon(ManagedIdentityTestConstants.RESOURCE).get();
 
@@ -697,8 +697,8 @@ void managedIdentityTest_RetriesARePerRequest() throws Exception {
 
             // First call returns 500, subsequent calls return 200
             when(httpClientMock.send(expectedRequest(IMDS, ManagedIdentityTestConstants.RESOURCE)))
-                    .thenReturn(expectedResponse(500, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500))
-                    .thenReturn(expectedResponse(200, getSuccessfulResponse(ManagedIdentityTestConstants.RESOURCE)));
+                    .thenReturn(expectedResponse(HttpStatus.INTERNAL_SERVER_ERROR.getCode(), ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500))
+                    .thenReturn(expectedResponse(HttpStatus.OK.getCode(), getSuccessfulResponse(ManagedIdentityTestConstants.RESOURCE)));
 
             IAuthenticationResult result = acquireTokenCommon(ManagedIdentityTestConstants.RESOURCE).get();
 
@@ -710,7 +710,7 @@ void managedIdentityTest_RetriesARePerRequest() throws Exception {
 
             //All calls return 500
             when(httpClientMock.send(expectedRequest(IMDS, "otherResource")))
-                    .thenReturn(expectedResponse(500, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
+                    .thenReturn(expectedResponse(HttpStatus.INTERNAL_SERVER_ERROR.getCode(), ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));
 
             CompletableFuture<IAuthenticationResult> future = acquireTokenCommon("otherResource");