Repository navigation
[Credentials Cache] retry credential invalidaiton errors + re-resolve identity per attempt #7406
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,11 +21,14 @@ | |
| import java.time.Duration; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.Executor; | ||
| import java.util.concurrent.RejectedExecutionException; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.TimeUnit; | ||
| import software.amazon.awssdk.annotations.SdkInternalApi; | ||
| import software.amazon.awssdk.core.Response; | ||
| import software.amazon.awssdk.core.async.AsyncRequestBody; | ||
| import software.amazon.awssdk.core.client.config.SdkAdvancedAsyncClientOption; | ||
| import software.amazon.awssdk.core.client.config.SdkClientOption; | ||
| import software.amazon.awssdk.core.exception.SdkException; | ||
| import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute; | ||
|
|
@@ -51,6 +54,7 @@ public final class AsyncRetryableStage<OutputT> implements RequestPipeline<SdkHt | |
| private final TransformingAsyncResponseHandler<Response<OutputT>> responseHandler; | ||
| private final RequestPipeline<SdkHttpFullRequest, CompletableFuture<Response<OutputT>>> requestPipeline; | ||
| private final ScheduledExecutorService scheduledExecutor; | ||
| private final Executor futureCompletionExecutor; | ||
| private final HttpClientDependencies dependencies; | ||
|
|
||
| public AsyncRetryableStage(TransformingAsyncResponseHandler<Response<OutputT>> responseHandler, | ||
|
|
@@ -59,6 +63,8 @@ public AsyncRetryableStage(TransformingAsyncResponseHandler<Response<OutputT>> r | |
| this.responseHandler = responseHandler; | ||
| this.dependencies = dependencies; | ||
| this.scheduledExecutor = dependencies.clientConfiguration().option(SdkClientOption.SCHEDULED_EXECUTOR_SERVICE); | ||
| this.futureCompletionExecutor = | ||
| dependencies.clientConfiguration().option(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR); | ||
| this.requestPipeline = requestPipeline; | ||
| } | ||
|
|
||
|
|
@@ -116,6 +122,7 @@ private void attemptExecute(CompletableFuture<Response<OutputT>> future) { | |
| CompletableFuture<Response<OutputT>> responseFuture; | ||
| try { | ||
| retryableStageHelper.startingAttempt(); | ||
| retryableStageHelper.resolveIdentityForAttempt(); | ||
|
Contributor
There was a problem hiding this comment. On async clients, retries reach this line through scheduleOrFail (line 187), so every retry resolves identity on an sdk-ScheduledExecutor thread. The default scheduler has 5 threads. It also runs the apiCallTimeout timers, the apiCallAttemptTimeout timers, and the retry backoff timers for every request on the client. If 5 or more requests retry an ExpiredToken at the same time, will the timeouts and retries of unrelated requests fire late until the refresh completes?
Contributor
Author
There was a problem hiding this comment. Yeah - this is a good callout. I was struggling with where/how to run the refresh on the async path. I went with the scheduler thread since it was already doing things like signing... but you are correct that a blocked/slow refresh would impact the api call timeout. I've moved all of the retry execution into the futureCompletionExecutor which I think is the more correct place for all of that retry loop logic to live. That fixes the issue with the apiCallTimeout (but does cost one more thread jump, but I think its logically more correct for all of the retry loop to execute outside of the scheduler threads). |
||
| retryableStageHelper.logSendingRequest(); | ||
| responseFuture = requestPipeline.execute(retryableStageHelper.requestToSend(), context); | ||
|
|
||
|
|
@@ -183,13 +190,34 @@ public void maybeAttemptExecute(CompletableFuture<Response<OutputT>> future) { | |
| Duration successDelay = backoffDelay.left().get(); | ||
| retryableStageHelper.logBackingOff(successDelay); | ||
| long totalDelayMillis = successDelay.toMillis(); | ||
| scheduleOrFail(() -> attemptRetryExecute(future), totalDelayMillis, MILLISECONDS, future); | ||
| } catch (Throwable t) { | ||
| future.completeExceptionally(t); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Runs a retry attempt on the future completion executor, off the scheduled executor that runs the client's timers. | ||
| * A retry attempt resolves the identity again, which can block while credentials are refreshed. Falls back to the | ||
| * current thread if there is no future completion executor or it rejects the attempt. | ||
| */ | ||
| private void attemptRetryExecute(CompletableFuture<Response<OutputT>> future) { | ||
| if (futureCompletionExecutor == null) { | ||
| attemptExecute(future); | ||
| return; | ||
| } | ||
| try { | ||
| futureCompletionExecutor.execute(() -> attemptExecute(future)); | ||
| } catch (RejectedExecutionException e) { | ||
| LOG.debug(() -> String.format("Could not run the retry attempt on the provided FUTURE_COMPLETION_EXECUTOR. " | ||
| + "The attempt will run on thread %s. This may be an indication that the " | ||
| + "executor is being overwhelmed by too many requests.", | ||
| Thread.currentThread().getName()), e); | ||
| attemptExecute(future); | ||
| } | ||
| } | ||
|
|
||
| private void maybeRetryExecute(CompletableFuture<Response<OutputT>> future, Exception exception) { | ||
| retryableStageHelper.setLastException(exception); | ||
| try { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when these two codes are retried on these paths?
On paths 1 and 2, AwsRequestIdentityProviderResolver (line 68) replaces the provider with StaticCredentialsProvider, whose invalidate() is a no-op. On path 3, the credentials are in the URL. Since the retry attempt signs with the same credentials (or sends the same URL), will these calls make up to 3 extra attempts with the default retry mode and still end with ExpiredToken, where before this change they failed after 1 attempt
WDYT about skipping the retry for these two codes when the identity can't change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah these are valid - each of these cases will result in new retries that are unlikely to succeed, but thats the cross-SDK spec for them.
I don't think we can reliably detect "identities that can't change" and the retry spec requires that we retry them unconditionally....