Allow overriding the default ClientFactory via FlagsProvider#6671
Allow overriding the default ClientFactory via FlagsProvider#6671JAEKWANG97 wants to merge 3 commits intoline:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java`:
- Around line 378-381: The current defaultClientFactory() creates a new
DefaultClientFactory each call causing the static DefaultClientFactory.DEFAULT
to never be closed; change DefaultFlagsProvider.defaultClientFactory() to return
the existing static instance (DefaultClientFactory.DEFAULT or
DefaultClientFactory.INSECURE as appropriate) instead of
ClientFactory.builder().build(), or alternatively ensure
DefaultClientFactory.DEFAULT is closed from ClientFactory.closeDefault();
specifically update the DefaultFlagsProvider.defaultClientFactory() method to
return the shared DefaultClientFactory.DEFAULT/INSECURE instances and remove
per-call builder() usage so the shutdown hook (ClientFactory.closeDefault())
will actually close the static DEFAULT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2090aaec-6248-45f1-88ec-336f039e0060
📒 Files selected for processing (7)
core/src/main/java/com/linecorp/armeria/client/ClientFactory.javacore/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/BaseFlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
|
While working on this, I also explored a broader draft that touched follow-up lifecycle-related behaviors such as Since those expanded the scope significantly, I narrowed this PR to focus on the default override mechanism itself and am opening it as a draft first. That broader exploration also suggested there may be room for structural cleanup around the ownership boundary between For reference, the earlier broader draft is here: I'd appreciate feedback on whether those lifecycle-related behaviors should be handled in follow-up changes, and whether that kind of structural cleanup would be preferred before expanding the scope. |
The overall changes look good. Could you explain in more detail what issues there are with the lifecycle of |
|
I'm unsure if Perhaps a saner approach may be to define a Long term, we'll probably need a separate mechanism for flag loading that doesn't rely on class initialization ordering (e.g. dependency injection) |
@ikhoon What I had in mind there was that the ownership of the default Concretely:
The main follow-up issues I ran into in the draft PR I closed were:
So the concern I had was less that any one class is individually wrong, and more that once the effective default is resolved through That was the context behind my comment about possible structural cleanup between |
|
@jrhee17 Agreed. The root issue is that Rather than the What do you think? |
|
I think I need to understand exactly what the issue is: So you are saying even if |
|
@jrhee17 I think I focused more on the lifecycle/ownership concern than on your initialization-order point, so let me address that first. I have not identified a concrete initialization failure in this draft so far. With That said, I agree with the broader concern that the current design still depends on Java class-initialization ordering, since So a Would it make sense to proceed in that direction? |
I think this is an issue though - they should refer to the same object. I don't think changing the lifecycle of flags is trivial, we may want to design this separately if needed. If the CI fails, we can analyze together and talk about whether/how this can be fixed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java (1)
225-229: Minor:setAccessible(true)is unnecessary for public methods.The
setAccessible(true)call is redundant when invoking public methods viagetMethod(). However, this is a harmless nit.♻️ Optional simplification
private static Object invokePublic(Object target, String methodName) throws Throwable { final Method method = target.getClass().getMethod(methodName); - method.setAccessible(true); return method.invoke(target); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java` around lines 225 - 229, The helper method invokePublic unnecessarily calls method.setAccessible(true) for a public method retrieved via getMethod(); remove the setAccessible(true) invocation from invokePublic (the Method method variable) so the method simply obtains the Method via target.getClass().getMethod(methodName) and invokes it, keeping behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java`:
- Around line 225-229: The helper method invokePublic unnecessarily calls
method.setAccessible(true) for a public method retrieved via getMethod(); remove
the setAccessible(true) invocation from invokePublic (the Method method
variable) so the method simply obtains the Method via
target.getClass().getMethod(methodName) and invokes it, keeping behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 556444f3-960b-4e2c-964f-597d629059e7
📒 Files selected for processing (8)
core/src/main/java/com/linecorp/armeria/client/ClientFactory.javacore/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.javait/flags-provider/src/test/java/com/linecorp/armeria/common/BaseFlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
- core/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.java
- core/src/main/java/com/linecorp/armeria/client/ClientFactory.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6671 +/- ##
============================================
- Coverage 74.46% 73.95% -0.51%
- Complexity 22234 24044 +1810
============================================
Files 1963 2171 +208
Lines 82437 90196 +7759
Branches 10764 11828 +1064
============================================
+ Hits 61385 66707 +5322
- Misses 15918 17893 +1975
- Partials 5134 5596 +462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java (1)
228-246: Consider adding cleanup protection for test robustness.Unlike
overrideDefaultClientFactory(), this test lacks a try-finally to ensurecloseDefault()is called if an assertion fails before line 243. This could leave resources open in failure scenarios, potentially affecting subsequent tests.♻️ Suggested fix to add try-finally for cleanup
`@Test` `@SetSystemProperty`(key = "com.linecorp.armeria.test.decoratingDefaultClientFactory", value = "true") void closeDefaultClosesDecoratingClientFactory() throws Throwable { final Method defaultClientFactoryMethod = flags.getDeclaredMethod("defaultClientFactory"); final Object clientFactory = defaultClientFactoryMethod.invoke(null); final Class<?> clientFactoryClass = defaultClientFactoryMethod.getReturnType(); assertThat(clientFactory.getClass().getSimpleName()).isEqualTo("TestDecoratingClientFactory"); final CompletableFuture<?> whenClosed = (CompletableFuture<?>) invokePublic(clientFactory, "whenClosed"); - assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(false); - invokePublic(clientFactory, "close"); - assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(false); - - invokeDeclaredStatic(clientFactoryClass, "closeDefault"); - whenClosed.join(); - assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(true); + try { + assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(false); + invokePublic(clientFactory, "close"); + assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(false); + + invokeDeclaredStatic(clientFactoryClass, "closeDefault"); + whenClosed.join(); + assertThat(invokePublic(clientFactory, "isClosed")).isEqualTo(true); + } finally { + if (!whenClosed.isDone()) { + invokeDeclaredStatic(clientFactoryClass, "closeDefault"); + whenClosed.join(); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java` around lines 228 - 246, Wrap the body of closeDefaultClosesDecoratingClientFactory (the code that invokes defaultClientFactory, checks isClosed, calls close, and asserts) in a try-finally so that invokeDeclaredStatic(clientFactoryClass, "closeDefault") is always executed in the finally block (and ensure whenClosed.join() is called to wait for closure), preserving any thrown assertion exception (i.e., do not swallow exceptions in the finally).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java`:
- Around line 228-246: Wrap the body of
closeDefaultClosesDecoratingClientFactory (the code that invokes
defaultClientFactory, checks isClosed, calls close, and asserts) in a
try-finally so that invokeDeclaredStatic(clientFactoryClass, "closeDefault") is
always executed in the finally block (and ensure whenClosed.join() is called to
wait for closure), preserving any thrown assertion exception (i.e., do not
swallow exceptions in the finally).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c250394a-1edd-4751-95de-c285d5d2a3e8
📒 Files selected for processing (3)
core/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.javait/flags-provider/src/test/java/com/linecorp/armeria/common/BaseFlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.java
I thought I'm not sure whether it is technically impossible to handle the side effects caused by the initialization order. If you are concerned about it, what do you think about returning |
I see - my original preference was that Since it seems like you have a idea of how this PR should move forward, I think it's probably better I revisit later when the PR is in a more reviewable state. |
|
I think the configurer/customizer direction you suggested seems simpler here. It keeps The tradeoff is narrowing the SPI from "full override" to "customizing the built-in default". If that still matches the intended scope of #6425, I’ll rework the PR in that direction unless there are objections. |
👍 I think it would be fine to process that way. |
JAEKWANG97
force-pushed
the
feature/override-default-client-factory-minimal
branch
from
e6b1b9e to
55fbebf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
1638-1652: Memoize the resolved configurator to keep the flag stable.Unlike most
Flagsaccessors, this re-runs provider resolution on every call. If a provider returns a stateful or newly-created configurator,ofDefault()andinsecure()can be built with different effective configuration depending on initialization timing.♻️ Proposed lazy memoization
`@UnstableApi` public static ClientFactoryConfigurator defaultClientFactoryConfigurator() { + return DefaultClientFactoryConfiguratorHolder.INSTANCE; + } + + private static final class DefaultClientFactoryConfiguratorHolder { + static final ClientFactoryConfigurator INSTANCE = defaultClientFactoryConfigurator0(); + } + + private static ClientFactoryConfigurator defaultClientFactoryConfigurator0() { final ClientFactoryConfigurator configurator = getValue(FlagsProvider::defaultClientFactoryConfigurator, "defaultClientFactoryConfigurator"); return configurator != null ? configurator : ClientFactoryConfigurator.noop(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/common/Flags.java` around lines 1638 - 1652, The getter defaultClientFactoryConfigurator() currently resolves the provider on every call causing instability; change it to lazily memoize the resolved ClientFactoryConfigurator in a private static volatile field (initialized once on first call) and return the cached instance thereafter (falling back to ClientFactoryConfigurator.noop() if null), ensuring thread-safe publication so ofDefault() and insecure() see a stable configurator; locate uses of getValue(FlagsProvider::defaultClientFactoryConfigurator, "defaultClientFactoryConfigurator") inside defaultClientFactoryConfigurator() to replace with the memoized lookup and assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/linecorp/armeria/client/ClientFactoryConfigurator.java`:
- Around line 1-15: Replace the current file header in
ClientFactoryConfigurator.java with the repository's standard copyright header
(remove the extra spaces after the '*' characters and restore the canonical
template form including the unchanged $today.year placeholder), ensuring the
header exactly matches other source files in the project so formatting and the
year placeholder are consistent.
---
Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/common/Flags.java`:
- Around line 1638-1652: The getter defaultClientFactoryConfigurator() currently
resolves the provider on every call causing instability; change it to lazily
memoize the resolved ClientFactoryConfigurator in a private static volatile
field (initialized once on first call) and return the cached instance thereafter
(falling back to ClientFactoryConfigurator.noop() if null), ensuring thread-safe
publication so ofDefault() and insecure() see a stable configurator; locate uses
of getValue(FlagsProvider::defaultClientFactoryConfigurator,
"defaultClientFactoryConfigurator") inside defaultClientFactoryConfigurator() to
replace with the memoized lookup and assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55b328ab-7ae9-4f83-b4e1-22c88291e259
📒 Files selected for processing (7)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryConfigurator.javacore/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/BaseFlagsProvider.javait/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
- core/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.java
- core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
- it/flags-provider/src/test/java/com/linecorp/armeria/common/BaseFlagsProvider.java
|
Thanks for the suggestion! I reworked the PR in the configurator direction. :) The current version keeps |
3 participants
Motivation:
The built-in default
ClientFactoryis currently fixed, so applications cannot customize default client entry points such asWebClient.of().Rather than overriding the default
ClientFactoryinstance itself, this change narrows the scope to customizing the built-in default client factories throughFlagsProvider.Modifications:
FlagsProvider.defaultClientFactoryConfigurator().Flags.defaultClientFactoryConfigurator().ClientFactoryConfiguratorfor customizingClientFactoryBuilder.DefaultClientFactory.DEFAULTandDefaultClientFactory.INSECURE.ClientFactory.ofDefault()returningDefaultClientFactory.DEFAULT.closeDefault(), directclose(), and shutdown hook ownership unchanged.FlagsProviderTest.Result:
FlagsProvider.ClientFactory.ofDefault()pick up the customized settings.Closes #6425.