Add AsyncLoader to load and update value periodically #5590

injae-kim · 2024-04-10T17:22:32Z

Motivation:

AsyncLoader can be useful in the following situations.

When it is necessary to periodically read and update information from a file such as resolv.conf .
When data is not valid after a certain period of time, such as an OAuth 2.0 access token.

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:

Add AsyncLoader to load and update value periodically

Result:

Closes Provide a way to load a value periodically or when it expires #5506.
Now user can load, cache, update value periodically by using AsyncLoader

Fixes line#5506.

github-actions · 2024-04-10T19:03:04Z

🔍 Build Scan® (commit: `cd45b6d`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/fssyk347qyr56
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/l2dd376pwgzz2
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/wlb4nbl57p4iu
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/udjvgppzpl6sg
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/fnvhd5iw5fxqk
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/jpsohnsz2lclc
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/rtbimclq3gv6k
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/vefzytepq7tuq

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

ikhoon · 2024-04-12T10:00:12Z

...2/src/main/java/com/linecorp/armeria/client/auth/oauth2/DefaultOAuth2AuthorizationGrant.java

+            if (token == null && fallbackTokenProvider != null) {
+                CompletableFuture<? extends GrantedOAuth2AccessToken> fallbackTokenFuture = null;
+                try {
+                    fallbackTokenFuture = requireNonNull(
+                            fallbackTokenProvider.get(), "fallbackTokenProvider.get() returned null");


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.

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

codecov · 2024-04-24T17:20:00Z

Codecov Report

Attention: Patch coverage is 84.31373% with 24 lines in your changes missing coverage. Please review.

Project coverage is 74.07%. Comparing base (b8eb810) to head (cca5962).
Report is 236 commits behind head on main.

❗ Current head cca5962 differs from pull request most recent head cd45b6d

Please upload reports for the commit cd45b6d to get more accurate results.

Files	Patch %	Lines
...t/auth/oauth2/DefaultOAuth2AuthorizationGrant.java	42.30%	13 Missing and 2 partials ⚠️
...necorp/armeria/common/util/DefaultAsyncLoader.java	94.33%	2 Missing and 4 partials ⚠️
...necorp/armeria/common/util/AsyncLoaderBuilder.java	85.00%	3 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

ikhoon

Overall looks good.

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

...2/src/main/java/com/linecorp/armeria/client/auth/oauth2/DefaultOAuth2AuthorizationGrant.java

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

.../src/test/java/com/linecorp/armeria/client/auth/oauth2/OAuth2ClientCredentialsGrantTest.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

minwoox

Looks great! 👍
Left a few comments.

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

core/src/test/java/com/linecorp/armeria/server/websocket/DelegatingWebSocketServiceTest.java

minwoox · 2024-06-05T02:19:19Z

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

+        }
+
+        @Override
+        public CacheEntry<U> join() {


Question: Do we need to override this method? cacheEntry is set to the result of the completable future anyway?

oh~ I found that we don't need to override this join() cause we call obtrudeValue(), obtrudeException() anyway inside of overrided complete(), completeExceptionally() 👍 thanks!

core/src/test/java/com/linecorp/armeria/common/util/DefaultAsyncLoaderTest.java

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

core/src/test/java/com/linecorp/armeria/server/websocket/DelegatingWebSocketServiceTest.java

ikhoon · 2024-06-05T06:55:10Z

core/src/test/java/com/linecorp/armeria/server/websocket/DelegatingWebSocketServiceTest.java

@@ -27,6 +27,7 @@
 import org.reactivestreams.Subscription;

 import com.linecorp.armeria.client.BlockingWebClient;
+import com.linecorp.armeria.client.WebClient;


Suggested change

import com.linecorp.armeria.client.WebClient;

95592de oh I simply remove this change~! 🙇

ikhoon

Looks nice, @injae-kim! ❤️

Please address @minwoox comments.

trustin · 2024-06-06T13:53:39Z

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

+     * Returns a newly created {@link AsyncLoaderBuilder} with the specified loader.
+     * @param loader function to load value. {@code T} is previously cached value


Let's insert an empty line between method description and parameter list for readability.

Suggested change

* Returns a newly created {@link AsyncLoaderBuilder} with the specified loader.

* @param loader function to load value. {@code T} is previously cached value

* Returns a newly created {@link AsyncLoaderBuilder} with the specified loader.

*

* @param loader function to load value. {@code T} is previously cached value

trustin · 2024-06-06T13:54:25Z

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java

+ * AsyncLoader<String> asyncLoader =
+ *     AsyncLoader
+ *         .builder(loader)
+ *         .expireAfterLoad(Duration.ofSeconds(60))  // The loaded value is expired after 60 seconds.


Let's make sure the line is not too long for readability.

Suggested change

* .expireAfterLoad(Duration.ofSeconds(60)) // The loaded value is expired after 60 seconds.

* // Expire the loaded value after 60 seconds.

* .expireAfterLoad(Duration.ofSeconds(60))

trustin · 2024-06-06T13:58:09Z

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

+ * A builder for creating a new {@link AsyncLoader}.
+ *
+ * <p>Expiration should be set by {@link #expireAfterLoad(Duration)} or {@link #expireIf(Predicate)}.
+ * If expiration is not set, {@link #build()} will throw {@link IllegalStateException}.


What do you think about not throwing an ISE but just building an instance that never expires its value?

trustin · 2024-06-06T13:59:18Z

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

+     * Expires The loaded value after the duration since it was loaded.
+     * New value will be loaded by the loader function on next {@link AsyncLoader#get()}.
+     */
+    public AsyncLoaderBuilder<T> expireAfterLoad(Duration expireAfterLoad) {


Maybe we could add the expireAfterLoadMillis(long) shortcut?

trustin · 2024-06-06T14:03:43Z

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

+    private static class CacheEntry<T> {
+
+        private final T value;
+        private final long cachedAt = System.nanoTime();


cachedAtNanos?

trustin · 2024-06-06T14:11:21Z

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

+        if (cacheEntry == null) {
+            return false;
+        }


Why is null an invalid entry value? A user might want to cache null as the result.

A user might want to cache null as the result.

I guess cacheEntry.value would be null in this case

trustin · 2024-06-06T14:17:29Z

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

+        }
+    }
+
+    private static class RefreshingFuture<U> extends CompletableFuture<CacheEntry<U>> {


We should not allow a user to change the state of this future via complete*() and obtrude*().

Can we create a new future when refreshing a new value, rather than using obtrude*()? I wouldn't risk using obtrude*() here because:

obtrude*() is designed by error recovery actions, which is not the case here

The future returned by get() can be reused by a user later; and

We can do without it.

jrhee17

Looks good in terms of correctness 👍 Thanks @injae-kim 🙇 👍 🙇

jrhee17 · 2024-06-07T08:58:22Z

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java

+     * @throws IllegalStateException if no expiration is set.
+     */
+    public AsyncLoader<T> build() {
+        if (expireAfterLoad == null && expireIf == null) {


Optional) Not too strong on this, but if CacheEntry were public then I guess it's possible to just remove expireAfterLoad and use expireIf to implement the same functionality.

This way would eliminate the ambiguity on which condition is evaluated first as well.

e.g.

AsyncLoaderBuilder<T> expireIf(Predicate<CacheEntry<? super T>> expireIf) {...} ... public AsyncLoaderBuilder<T> expireAfterLoad(Duration expireAfterLoad) { ... expireIf = cacheEntry -> { final long elapsed = System.nanoTime() - cacheEntry.cachedAt(); return elapsed < expireAfterLoad.toNanos(); };

jrhee17 · 2024-06-07T10:13:17Z

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java

+        if (cacheEntry == null) {
+            return false;
+        }


A user might want to cache null as the result.

I guess cacheEntry.value would be null in this case

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 10, 2024 17:22

Add AsyncLoader to load and update value periodically

e050aaa

Fixes line#5506.

injae-kim force-pushed the async-loader branch from cf8056a to e050aaa Compare April 10, 2024 17:31

injae-kim mentioned this pull request Apr 10, 2024

Provide a way to load a value periodically or when it expires #5506

Open

injae-kim commented Apr 12, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java Show resolved Hide resolved

ikhoon added new feature sprint Issues for OSS Sprint participants labels Apr 12, 2024

ikhoon reviewed Apr 12, 2024

View reviewed changes

Address comments

39dc307

injae-kim force-pushed the async-loader branch 3 times, most recently from 526db63 to e161fcc Compare April 17, 2024 18:28

injae-kim requested a review from ikhoon April 18, 2024 04:22

injae-kim force-pushed the async-loader branch from e161fcc to 95c1ab1 Compare April 21, 2024 15:12

ikhoon reviewed Apr 22, 2024

View reviewed changes

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

injae-kim added 2 commits April 23, 2024 01:36

Add exceptionHandler

fcad899

Add refreshIf for pre-fetch

ea412c7

injae-kim force-pushed the async-loader branch from 95c1ab1 to ea412c7 Compare April 23, 2024 17:44

injae-kim added 2 commits April 25, 2024 01:12

Refresh before check isValid to resolve timing issue

887d4a4

Add refresh failure log and enhance test

cca5962

injae-kim added 2 commits April 26, 2024 00:04

Throws IllegalStateException when expiration is not set on builder

4d917a1

Fix refresh to load value before return

97861bf

ikhoon reviewed Apr 26, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java Outdated Show resolved Hide resolved

Introduce RefreshingFuture to easily handle refreshIf

5b88969

injae-kim commented Apr 28, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java Outdated Show resolved Hide resolved

injae-kim commented Apr 28, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java Outdated Show resolved Hide resolved

ikhoon reviewed May 8, 2024

View reviewed changes

injae-kim added 4 commits May 8, 2024 23:33

Remove refreshExecutor

2b46072

Address comments

4607a6b

Make tokenLoader inside of DefaultOAuth2AuthorizationGrant constructor

6675ad5

Fix test

fb5d2ce

injae-kim commented May 9, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java Show resolved Hide resolved

injae-kim commented May 9, 2024

View reviewed changes

.../src/test/java/com/linecorp/armeria/client/auth/oauth2/OAuth2ClientCredentialsGrantTest.java Show resolved Hide resolved

trustin reviewed May 9, 2024

View reviewed changes

Address comments

af5fea9

ikhoon reviewed May 28, 2024

View reviewed changes

Address @ikhoon's comment

6536909

ikhoon added this to the 1.29.0 milestone Jun 4, 2024

ikhoon added 3 commits June 4, 2024 12:13

clean up

8533294

clean up 2

e5ba07d

Apply refreshIf to DefaultOAuth2AuthorizationGrant and add tests

290ed7f

minwoox reviewed Jun 5, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/AsyncLoader.java Outdated Show resolved Hide resolved

injae-kim added 2 commits June 5, 2024 15:03

Fix lint

aa5fb87

Address @minwoox's comment

416075e

ikhoon reviewed Jun 5, 2024

View reviewed changes

ikhoon approved these changes Jun 5, 2024

View reviewed changes

injae-kim force-pushed the async-loader branch from dc39ba7 to d6a4442 Compare June 5, 2024 07:09

Fix test

5282903

injae-kim force-pushed the async-loader branch from d6a4442 to 5282903 Compare June 5, 2024 07:10

injae-kim added 2 commits June 5, 2024 16:15

Revert @ikhoon's change

95592de

Remove unnecessary override join()

cd45b6d

trustin reviewed Jun 6, 2024

View reviewed changes

jrhee17 approved these changes Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AsyncLoader to load and update value periodically #5590

Add AsyncLoader to load and update value periodically #5590

injae-kim commented Apr 10, 2024

github-actions bot commented Apr 10, 2024 •

edited

ikhoon Apr 12, 2024

codecov bot commented Apr 24, 2024 •

edited

ikhoon left a comment

minwoox left a comment

minwoox Jun 5, 2024

injae-kim Jun 5, 2024 •

edited

ikhoon Jun 5, 2024

injae-kim Jun 5, 2024

ikhoon left a comment

trustin Jun 6, 2024

trustin Jun 6, 2024

trustin Jun 6, 2024

trustin Jun 6, 2024

trustin Jun 6, 2024

trustin Jun 6, 2024

jrhee17 Jun 7, 2024

trustin Jun 6, 2024

jrhee17 left a comment

jrhee17 Jun 7, 2024

jrhee17 Jun 7, 2024

		* Returns a newly created {@link AsyncLoaderBuilder} with the specified loader.
		* @param loader function to load value. {@code T} is previously cached value

	* .expireAfterLoad(Duration.ofSeconds(60)) // The loaded value is expired after 60 seconds.
	* // Expire the loaded value after 60 seconds.
	* .expireAfterLoad(Duration.ofSeconds(60))

Add AsyncLoader to load and update value periodically #5590

Are you sure you want to change the base?

Add AsyncLoader to load and update value periodically #5590

Conversation

injae-kim commented Apr 10, 2024

Motivation:

Modifications:

Result:

github-actions bot commented Apr 10, 2024 • edited

🔍 Build Scan® (commit: cd45b6d)

Choose a reason for hiding this comment

codecov bot commented Apr 24, 2024 • edited

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

injae-kim Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2024 •

edited

🔍 Build Scan® (commit: `cd45b6d`)

codecov bot commented Apr 24, 2024 •

edited

injae-kim Jun 5, 2024 •

edited