Add AsyncLoader to load and update value periodically#5590
Add AsyncLoader to load and update value periodically#5590
Conversation
| if (token == null && fallbackTokenProvider != null) { | ||
| CompletableFuture<? extends GrantedOAuth2AccessToken> fallbackTokenFuture = null; | ||
| try { | ||
| fallbackTokenFuture = requireNonNull( | ||
| fallbackTokenProvider.get(), "fallbackTokenProvider.get() returned null"); |
There was a problem hiding this comment.
Side note: It is not related to this PR but fallbackTokenProvider is a name that we should only use when a token acquisition fails since it is a fallback.
I think it would be better to remove fallback from the API method. tokenProvider seems clearer.
526db63 to
e161fcc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5590 +/- ##
============================================
+ Coverage 73.95% 74.07% +0.11%
- Complexity 20115 21252 +1137
============================================
Files 1730 1848 +118
Lines 74161 78567 +4406
Branches 9465 10024 +559
============================================
+ Hits 54847 58199 +3352
- Misses 14837 15669 +832
- Partials 4477 4699 +222 ☔ View full report in Codecov by Sentry. |
ikhoon
left a comment
There was a problem hiding this comment.
Looks nice, @injae-kim! ❤️
Please address @minwoox comments.
jrhee17
left a comment
There was a problem hiding this comment.
Looks good in terms of correctness 👍 Thanks @injae-kim 🙇 👍 🙇
- Remove RefreshingFuture and set CacheEntry as a field - Allow null value. - Allow no expiration.
minwoox
left a comment
There was a problem hiding this comment.
Look great! Left a small suggestion. 👍
| if (loadFutureUpdater.compareAndSet(this, loadFuture, future)) { | ||
| needsLoad = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
What do you think of returing this.loadFuture here? If so we can remove the for loop and reduce accessing the volatile fields.
There was a problem hiding this comment.
This is what I've imagined:
@Override
public CompletableFuture<T> load() {
final CompletableFuture<T> loadFuture = this.loadFuture;
if (!loadFuture.isDone()) {
// A load is in progress.
return returnCacheOrFuture(cacheEntry, loadFuture);
}
final CacheEntry<T> cacheEntry = this.cacheEntry;
final boolean isValid = isValid(cacheEntry);
if (!isValid || needsRefresh(cacheEntry)) {
final CompletableFuture<T> newFuture = new CompletableFuture<>();
if (loadFutureUpdater.compareAndSet(this, loadFuture, newFuture)) {
final T cache = cacheEntry != null ? cacheEntry.value : null;
load(cache, newFuture);
if (isValid) {
logger.debug("Pre-fetching a new value. cache: {}, loader: {}", cache, loader);
return UnmodifiableFuture.completedFuture(cache);
}
logger.debug("Loading a new value. cache: {}, loader: {}", cache, loader);
return newFuture;
}
return returnCacheOrFuture(this.cacheEntry, this.loadFuture);
} else {
// The cache is still valid and no need to refresh.
return returnCacheOrFuture(cacheEntry, loadFuture);
}
}
private CompletableFuture<T> returnCacheOrFuture(@Nullable CacheEntry<T> cacheEntry,
CompletableFuture<T> loadFuture) {
if (isValid(cacheEntry)) {
return UnmodifiableFuture.completedFuture(cacheEntry.value);
}
return loadFuture;
}
minwoox
left a comment
There was a problem hiding this comment.
Looks great! Thanks a lot, @injae-kim and @ikhoon! 👍 👍 👍 👍 👍
|
@ikhoon really thank you for polishing PR & address comments 🙇 🙇 The code looks much simpler&nicer now! |
|
@jrhee17 The code has been changed after your approval. I was wondering if you want to check the code again or if you already have done. |
|
Gentle ping @trustin. The usage of |
I think it should be fine to go ahead as I don't think the changes are in a critical path 👍 |
Fixes #5506.
Motivation:
AsyncLoadercan be useful in the following situations.We already have an implementation for that on AbstractOAuth2AuthorizationGrant.java.
However, I hope to generalize it and add new features to use it in various cases.
Modifications:
AsyncLoaderto load and update value periodicallyResult:
AsyncLoader