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: `af5fea9`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/5qdqhqxcd72x4
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/gf3wc7ctus6lm
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/ygar2n3zo25am
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/bq65hkiihjmhq
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/a6y4blqyvr6u4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/jrjz6dmlpubvu
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/2tgf54u7flvds
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/jlklh4dsywtvq

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 are missing coverage. Please review.

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

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

Please upload reports for the commit af5fea9 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

injae-kim · 2024-05-09T01:27:19Z

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

+        private static final RefreshingFuture<?> COMPLETED;
+
+        static {
+            COMPLETED = new RefreshingFuture<>(null);
+            COMPLETED.complete(null);
+        }
+
+        @SuppressWarnings("unchecked")
+        static <T> RefreshingFuture<T> completedFuture() {
+            return (RefreshingFuture<T>) COMPLETED;
+        }


Inspired by UnmodifiableFuture.completedFuture implementation!

injae-kim · 2024-05-09T01:30:47Z

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


        final List<AggregatedHttpResponse> responses3 =
                getConcurrently(client, "/resource-read-write-update/", count).join();
        validateResponses(responses3, HttpStatus.FORBIDDEN);
-        verify(grant, times(1)).obtainAccessToken(any());
-        verify(grant, times(0)).refreshAccessToken(any(), any());
+        assertThat(newTokenCounter.get()).isOne();


DefaultOAuth2AuthorizationGrant(..) { tokenLoader = tokenLoader(); } FAILED: wanted but not invoked obtainAccessToken, only invoked getAccessToken();

We make tokenLoader in constructor, then mockito can't verify obtainAccessToken, refreshAccessToken invocation well.

So I replace it with newTokenCounter :)

trustin · 2024-05-09T06:23:00Z

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

+import com.linecorp.armeria.common.annotation.Nullable;
+
+/**
+ * An {@link AsyncLoader} to atomically load, cache and update value.


I think it doesn't make much sense to refer to itself when defining something, especially in the first sentence of the definition. Can we rephrase this?

✅ updated!

trustin · 2024-05-09T06:30:41Z

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

+                    if (!needsRefresh(cacheEntry) || loadFuture.refreshing) {
+                        return loadFuture;
+                    }


Maybe log here about why and why not refreshing?

✅ updated! add logger.debug(..)

trustin · 2024-05-09T06:31:24Z

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

+        }
+    }
+
+    private boolean isValid(@Nullable CacheEntry<T> cacheEntry) {


Maybe add more logging in this method to make it easy to figure why isValid() returned false?

✅ updated! add logger.debug(..)

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

trustin reviewed May 9, 2024

View reviewed changes

Address comments

af5fea9

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

injae-kim May 9, 2024

injae-kim May 9, 2024

trustin May 9, 2024

injae-kim May 19, 2024

trustin May 9, 2024

injae-kim May 19, 2024

trustin May 9, 2024

injae-kim May 19, 2024

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: af5fea9)

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

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

github-actions bot commented Apr 10, 2024 •

edited

🔍 Build Scan® (commit: `af5fea9`)

codecov bot commented Apr 24, 2024 •

edited