Skip to content

Commit

Permalink
OAuth2Client: defer initial token fetch when user interaction is requ…
Browse files Browse the repository at this point in the history
…ired (#8354)

Currently the initial token fetch occurs when `start()` is called. This is fine for grant types that do not need user input and speeds up the process so that tokens are hopefully available when the client is actually used the first time.

But there are 2 issues with this "early start":

* If user input is required, then a background thread will block waiting for the user; this thread cannot be interrupted.
* In case the fetch fails, confusing log messages are printed in the console (confusing because they happen before the user tries to use the client).
 
This PR thus defers the initial token fetch in case of user interaction until the user actually attempts to use the client.

It also defers printing any log messages until the client has been accessed for the first time.
  • Loading branch information
adutra committed Apr 18, 2024
1 parent d955f13 commit e6451c1
Showing 1 changed file with 15 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class OAuth2Client implements OAuth2Authenticator, Closeable {
private final HttpClient httpClient;
private final ScheduledExecutorService executor;
private final CompletableFuture<Void> started = new CompletableFuture<>();
private final CompletableFuture<Void> used = new CompletableFuture<>();
/* Visible for testing. */ final AtomicBoolean sleeping = new AtomicBoolean();
private final AtomicBoolean closing = new AtomicBoolean();

Expand All @@ -70,14 +71,21 @@ class OAuth2Client implements OAuth2Authenticator, Closeable {
.getExecutor()
.orElseGet(
() -> new OAuth2TokenRefreshExecutor(config.getBackgroundThreadIdleTimeout()));
currentTokensStage = started.thenApplyAsync((v) -> fetchNewTokens(), this.executor);
// when user interaction is not required, token fetch can happen immediately upon start();
// otherwise, it will be deferred until authenticate() is called the first time.
CompletableFuture<?> ready =
config.getGrantType().requiresUserInteraction()
? CompletableFuture.allOf(started, used)
: started;
currentTokensStage = ready.thenApplyAsync((v) -> fetchNewTokens(), executor);
currentTokensStage
.whenComplete((tokens, error) -> log(error))
.whenComplete((tokens, error) -> maybeScheduleTokensRenewal(tokens));
}

@Override
public AccessToken authenticate() {
used.complete(null);
Instant now = config.getClock().get();
lastAccess = now;
if (sleeping.compareAndSet(true, false)) {
Expand Down Expand Up @@ -136,6 +144,8 @@ public void close() {
// Always close the HTTP client (can't be shared).
httpClient.close();
} finally {
// Cancel this future to invalidate any pending log messages
used.cancel(true);
tokenRefreshFuture = null;
}
LOGGER.debug("Closed");
Expand Down Expand Up @@ -270,10 +280,12 @@ private void maybeWarn(String message, Throwable error) {
boolean shouldWarn =
lastWarn == null || Duration.between(lastWarn, now).compareTo(MIN_WARN_INTERVAL) > 0;
if (shouldWarn) {
// defer logging until the client is used to avoid confusing log messages appearing
// before the client is actually used
if (error instanceof HttpClientException) {
LOGGER.warn("{}: {}", message, error.toString());
used.thenRun(() -> LOGGER.warn("{}: {}", message, error.toString()));
} else {
LOGGER.warn(message, error);
used.thenRun(() -> LOGGER.warn(message, error));
}
lastWarn = now;
} else {
Expand Down

0 comments on commit e6451c1

Please sign in to comment.