-
Notifications
You must be signed in to change notification settings - Fork 933
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
Add AsyncLoader to load and update value periodically #5590
Conversation
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
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. |
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/server/websocket/DelegatingWebSocketServiceTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/server/websocket/DelegatingWebSocketServiceTest.java
Outdated
Show resolved
Hide resolved
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.
Looks nice, @injae-kim! ❤️
Please address @minwoox comments.
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
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.
Looks good in terms of correctness 👍 Thanks @injae-kim 🙇 👍 🙇
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
Outdated
Show resolved
Hide resolved
oauth2/src/test/java/com/linecorp/armeria/common/auth/oauth2/OAuth2RefreshBeforeTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Outdated
Show resolved
Hide resolved
- Remove RefreshingFuture and set CacheEntry as a field - Allow null value. - Allow no expiration.
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.
Look great! Left a small suggestion. 👍
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
Outdated
Show resolved
Hide resolved
if (loadFutureUpdater.compareAndSet(this, loadFuture, future)) { | ||
needsLoad = true; | ||
break; | ||
} |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
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.
It looks nicer.
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.
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:
AsyncLoader
can 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:
AsyncLoader
to load and update value periodicallyResult:
AsyncLoader