Skip to content

Add API to disable internal retry behavior #963

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

Merged
merged 3 commits into from
Jun 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public abstract class AbstractApplicationBase implements IApplicationBase {
private IHttpClient httpClient;
private Integer connectTimeoutForDefaultHttpClient;
private Integer readTimeoutForDefaultHttpClient;
private boolean retryDisabled;
String tenant;

//The following fields are set in only some applications and/or set internally by the library. To avoid excessive
Expand Down Expand Up @@ -150,6 +151,10 @@ public Integer readTimeoutForDefaultHttpClient() {
return this.readTimeoutForDefaultHttpClient;
}

boolean isRetryDisabled() {
return this.retryDisabled;
}

String tenant() {
return this.tenant;
}
Expand Down Expand Up @@ -190,6 +195,7 @@ public abstract static class Builder<T extends Builder<T>> {
Boolean onlySendFailureTelemetry = false;
Integer connectTimeoutForDefaultHttpClient;
Integer readTimeoutForDefaultHttpClient;
boolean disableInternalRetries;
private String clientId;
private Authority authenticationAuthority = createDefaultAADAuthority();

Expand Down Expand Up @@ -319,6 +325,18 @@ public T readTimeoutForDefaultHttpClient(Integer val) {
return self();
}

/**
* The library has a number of policies for retrying HTTP calls in different scenarios.
* <p>
* This will disable all internal retry behavior, allowing customized retry behavior via your own implementation of {@link IHttpClient}
*
* @return instance of the Builder on which method was called
*/
public T disableInternalRetries() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename it to disableMsalRetries. In MSAL.Net this is passed as a flag when passing HttpClientFactory. Would recommend to be consistent with that if possible. WithHttpClientFactory. The name of the flag is no longer valid in MSAL.Net but similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

disableInternalRetries = true;
return self();
}

T telemetryConsumer(Consumer<List<HashMap<String, String>>> val) {
validateNotNull("telemetryConsumer", val);

Expand Down Expand Up @@ -356,5 +374,16 @@ private static Authority createDefaultAADAuthority() {
readTimeoutForDefaultHttpClient = builder.readTimeoutForDefaultHttpClient;
authenticationAuthority = builder.authenticationAuthority;
clientId = builder.clientId;
retryDisabled = builder.disableInternalRetries;

if (builder.httpClient == null) {
httpClient = new DefaultHttpClient(
builder.proxy,
builder.sslSocketFactory,
builder.connectTimeoutForDefaultHttpClient,
builder.readTimeoutForDefaultHttpClient);
} else {
httpClient = builder.httpClient;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -566,10 +566,7 @@ public T correlationId(String val) {
super.serviceBundle = new ServiceBundle(
builder.executorService,
new TelemetryManager(telemetryConsumer, builder.onlySendFailureTelemetry),
new HttpHelper(builder.httpClient == null ?
new DefaultHttpClient(builder.proxy, builder.sslSocketFactory, builder.connectTimeoutForDefaultHttpClient, builder.readTimeoutForDefaultHttpClient) :
builder.httpClient,
new DefaultRetryPolicy())
new HttpHelper(this, new DefaultRetryPolicy())
);

if (aadAadInstanceDiscoveryResponse != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,19 @@ class HttpHelper implements IHttpHelper {

private IHttpClient httpClient;
private IRetryPolicy retryPolicy;
private boolean retryDisabled;

HttpHelper(IHttpClient httpClient, IRetryPolicy retryPolicy) {
this.httpClient = httpClient;
this.retryPolicy = retryPolicy != null ? retryPolicy : new DefaultRetryPolicy();
}

HttpHelper(AbstractApplicationBase application, IRetryPolicy retryPolicy) {
this.httpClient = application.httpClient();
this.retryDisabled = application.isRetryDisabled();
this.retryPolicy = retryPolicy != null ? retryPolicy : new DefaultRetryPolicy();
}

public IHttpResponse executeHttpRequest(HttpRequest httpRequest,
RequestContext requestContext,
ServiceBundle serviceBundle) {
Expand Down Expand Up @@ -145,6 +152,10 @@ IHttpResponse executeHttpRequestWithRetries(HttpRequest httpRequest, IHttpClient
throws Exception {
IHttpResponse httpResponse = httpClient.send(httpRequest);

if (retryDisabled) {
return httpResponse;
}

int retryCount = 0;
int maxRetries = retryPolicy.getMaxRetryCount(httpResponse);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,7 @@ private ManagedIdentityApplication(Builder builder) {
super.serviceBundle = new ServiceBundle(
builder.executorService,
new TelemetryManager(telemetryConsumer, builder.onlySendFailureTelemetry),
new HttpHelper(builder.httpClient == null ?
new DefaultHttpClient(builder.proxy, builder.sslSocketFactory, builder.connectTimeoutForDefaultHttpClient, builder.readTimeoutForDefaultHttpClient) :
builder.httpClient,
new ManagedIdentityRetryPolicy())
new HttpHelper(this, new ManagedIdentityRetryPolicy())
);
log = LoggerFactory.getLogger(ManagedIdentityApplication.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,40 @@ void managedIdentityTest_Retry(ManagedIdentitySourceType source, String endpoint
fail("MsalServiceException is expected but not thrown.");
}

@ParameterizedTest
@MethodSource("com.microsoft.aad.msal4j.ManagedIdentityTestDataProvider#createDataWrongScope")
void managedIdentityTest_RetriesDisabled(ManagedIdentitySourceType source, String endpoint, String resource) throws Exception {
IEnvironmentVariables environmentVariables = new EnvironmentVariablesHelper(source, endpoint);
ManagedIdentityApplication.setEnvironmentVariables(environmentVariables);

DefaultHttpClientManagedIdentity httpClientMock = mock(DefaultHttpClientManagedIdentity.class);
if (source == SERVICE_FABRIC) {
ServiceFabricManagedIdentitySource.setHttpClient(httpClientMock);
}

miApp = ManagedIdentityApplication
.builder(ManagedIdentityId.systemAssigned())
.httpClient(httpClientMock)
.disableInternalRetries()
.build();

//Several specific 4xx and 5xx errors, such as 500, should trigger MSAL's retry logic
when(httpClientMock.send(expectedRequest(source, resource))).thenReturn(expectedResponse(500, ManagedIdentityTestConstants.MSI_ERROR_RESPONSE_500));

try {
acquireTokenCommon(resource).get();
} catch (Exception exception) {
assert(exception.getCause() instanceof MsalServiceException);

//But because retries are disabled at the app level, there should be no retries for any source except Service Fabric, which uses its own HttpClient
if (source == SERVICE_FABRIC) {
verify(httpClientMock, times(4)).send(any());
} else {
verify(httpClientMock, times(1)).send(any());
}
}
}

@Test
void managedIdentityTest_IMDSRetry() throws Exception {
IEnvironmentVariables environmentVariables = new EnvironmentVariablesHelper(IMDS, ManagedIdentityTestConstants.IMDS_ENDPOINT);
Expand Down