fix: multiprovider parity gaps with JS SDK reference (#1882) - #1897
fix: multiprovider parity gaps with JS SDK reference (#1882)#1897jonathannorris wants to merge 8 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenFeature Java SDK's multiprovider capabilities, bringing its behavior and performance in line with the JavaScript SDK. The changes focus on improving how context is managed during hook execution, optimizing the parallel evaluation strategy to prevent resource issues, and standardizing error aggregation across multiple providers. These updates aim to create a more robust and consistent experience for developers using multiproviders. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the MultiProvider functionality by introducing a new ComparisonStrategy, improving event handling, and implementing a robust hook execution mechanism. The changes decouple OpenFeatureClient from MultiProvider by using a provider-level before hook for context capture, aligning with the JS SDK's approach. The ComparisonStrategy now reuses a shared executor, preventing resource exhaustion. Comprehensive state management for child providers and error aggregation have been added, along with extensive test coverage for concurrent evaluation, error handling, and hook context capture. The overall quality of the changes is high, with careful consideration for thread safety, error handling, and maintainability.
58843b2 to
05f0fce
Compare
|
/gemini review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1897 +/- ##
============================================
- Coverage 92.52% 90.76% -1.76%
- Complexity 728 846 +118
============================================
Files 60 63 +3
Lines 1739 2155 +416
Branches 202 258 +56
============================================
+ Hits 1609 1956 +347
- Misses 80 114 +34
- Partials 50 85 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR closes multi-provider parity gaps vs the JS SDK by improving hook/context handling, provider state/event aggregation, tracking forwarding, and adding a new parallel ComparisonStrategy, along with expanded test coverage.
Changes:
- Add provider-level context capture and per-child-provider hook execution with per-provider context isolation in
MultiProvider. - Add child provider state tracking + event observation/aggregation and
track()forwarding inMultiProvider(enabled by new observer APIs inEventProvider). - Introduce
ComparisonStrategyfor parallel evaluation with fallback-on-mismatch behavior, plus new/updated tests and Maven wrapper script.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java | Adds context-capturing provider hook, per-provider hook execution, provider deduplication, state aggregation, event observation, and track() forwarding. |
| src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java | New parallel comparison strategy with fallback provider + optional mismatch callback. |
| src/main/java/dev/openfeature/sdk/EventProvider.java | Adds observer registration/removal to support composite providers observing child provider events. |
| src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java | Expands duplicate-name test to assert deduped metadata entries are preserved. |
| src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java | New tests validating provider hook execution, isolation, hint/metadata propagation, and error enrichment. |
| src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java | New tests for state aggregation, config-event forwarding, init-time event preservation, and track() forwarding rules. |
| src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java | New tests for comparison behavior, concurrency, executor reuse, and aggregated error handling. |
| mvnw | Adds Maven wrapper shell script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the MultiProvider and EventProvider components, including a new ComparisonStrategy for parallel provider evaluation and result comparison. Key updates to MultiProvider include automated provider name deduplication, a state aggregation mechanism that propagates child provider events, and support for tracking event forwarding. Additionally, the implementation now includes logic for isolated hook execution for individual child providers. Review feedback identifies a high-severity issue in the hook execution lifecycle, noting that the current order of hook processing deviates from the OpenFeature specification.
|
Co-authored-by: jonathan <jonathan@taplytics.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
- Decouple OpenFeatureClient from MultiProvider by using a provider-level before hook for context capture (matching JS SDK WeakMap pattern) - Fix ComparisonStrategy to reuse shared ForkJoinPool.commonPool() instead of creating a new thread pool per evaluation call - Add timeout support and custom ExecutorService constructor to ComparisonStrategy - Fix checkstyle violations (Javadoc, line length, import ordering) - Improve type safety in normalizeDefaultValue - Add tests for concurrent evaluation, executor reuse, multi-error collection, and provider hook context capture Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…xecutorService field Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…from MultiProvider Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…mparisonStrategy Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
29f2e01 to
9504869
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java:74
- This reverses the lifecycle ordering that the same provider hooks receive when the provider is used directly.
HookSupport.executeBeforeHooksiterates hooks in reverse registration order (HookSupport.java:78-94), whileafter,error, andfinallyAfteriterate in registration order (HookSupport.java:97-135). Here the loops do the opposite, so merely wrapping a provider changes observable hook behavior. UsereversedHooksforbeforeandhooksfor the later stages, and update the ordering test accordingly.
// Per spec, before hooks run in registration order; after/error/finally run in reverse.
List<HookExecution<T>> reversedHooks = new ArrayList<>(hooks);
Collections.reverse(reversedHooks);
src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java:359
- This hard-coded state check bypasses the active
Strategy, so custom strategies cannot control which providers receive tracking as required by #1882. The referenced JS strategy API delegates this decision toshouldTrackWithThisProvider, with NOT_READY/FATAL filtering as its default. Extend the Java strategy contract with an equivalent predicate (including provider state and tracking inputs) and delegate this decision to it here.
if (!shouldTrackProvider(providerName)) {
src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java:168
- The fallback provider is documented as the choice when providers disagree, but this also returns its full result on agreement. Equal values can still carry different variant, reason, and metadata, so this diverges from the JS reference, which returns the first registered resolution when all values agree and uses the fallback only on mismatch. Return the first provider's result on this path and update the class documentation/test to distinguish the result details.
if (allEvaluationsMatch(successfulResults)) {
return fallbackResult;
src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java:144
- A timed-out provider is cancelled but never added to
providerErrors, so the returnedMultiProviderEvaluationcan contain an empty or partial error list even though the Strategy contract promises per-provider failure details. Keep each future associated with its provider name and add a timeoutProviderErrorfor every cancelled future before building the aggregate result.
if (future.isCancelled()) {
return errorResult(
"Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors);
| provider.initialize(evaluationContext, domain); | ||
| setProviderReadyIfStillNotReady(providerName); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java (1)
264-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated strategy plus hook-executor wiring into one generic helper.
The five evaluation methods differ only in the value type, the
FlagValueType, and the terminal provider call. The remaining eight-argument wiring is duplicated five times. A generic helper keeps the call shape in one place, so a future change to the hook-executor signature touches one method.♻️ Proposed refactor
+ private <T> ProviderEvaluation<T> evaluateWithHooks( + String key, + T defaultValue, + EvaluationContext ctx, + FlagValueType valueType, + BiFunction<FeatureProvider, EvaluationContext, ProviderEvaluation<T>> providerFunction) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); + return strategy.evaluate( + providers, + key, + defaultValue, + ctx, + provider -> hookExecutor.evaluate( + provider, key, defaultValue, ctx, hookExecutionContext, valueType, providerFunction)); + } + `@Override` public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { - HookExecutionContext hookExecutionContext = currentHookExecutionContext(); - return strategy.evaluate( - providers, - key, - defaultValue, - ctx, - provider -> hookExecutor.evaluate( - provider, - key, - defaultValue, - ctx, - hookExecutionContext, - FlagValueType.BOOLEAN, - (p, evaluationContext) -> p.getBooleanEvaluation(key, defaultValue, evaluationContext))); + return evaluateWithHooks( + key, + defaultValue, + ctx, + FlagValueType.BOOLEAN, + (p, evaluationContext) -> p.getBooleanEvaluation(key, defaultValue, evaluationContext)); }Apply the same replacement to
getStringEvaluation,getIntegerEvaluation,getDoubleEvaluation, andgetObjectEvaluation.java.util.function.BiFunctionneeds an import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java` around lines 264 - 352, Extract the duplicated strategy and hook-executor wiring from the five evaluation methods into one generic helper in MultiProvider, parameterized by value type, FlagValueType, and the terminal provider evaluation BiFunction. Update getBooleanEvaluation, getStringEvaluation, getIntegerEvaluation, getDoubleEvaluation, and getObjectEvaluation to delegate to that helper, and add the required BiFunction import.src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java (1)
78-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShut down the MultiProvider in these two tests.
shouldPreserveChildStateEmittedDuringInitializeandshouldForwardTrackToReadyProvidersAndSkipFatalProviderscallinitialize(null)but never callshutdown(). EachTrackingProviderand theMultiProviderkeep a cached emitter thread pool alive for the rest of the test JVM. Test 1 already isolates cleanup in afinallyblock; the same pattern here keeps the suite consistent.try { multiProvider.initialize(null); // assertions } finally { multiProvider.shutdown(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java` around lines 78 - 113, Update shouldPreserveChildStateEmittedDuringInitialize and shouldForwardTrackToReadyProvidersAndSkipFatalProviders to wrap initialization and assertions in finally blocks that call multiProvider.shutdown(). Preserve the existing test assertions while ensuring cleanup runs even when a test fails.src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java (1)
43-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
evaluateto clear the failing SonarCloud gate.SonarCloud fails this method with cognitive complexity 29 against a limit of 15, and it also flags the nested
tryat Line 117. The method repeats the same seven-argumentcreateHookContext(...)call four times and inlines four stage loops.Extract one context factory per execution and one method per stage. That removes both findings without changing behavior.
♻️ Suggested decomposition
private <T> HookContext<T> contextFor(HookExecution<T> execution, EvaluationContext evaluatedContext, ...) { return createHookContext(key, valueType, defaultValue, evaluatedContext, provider, hookExecutionContext, execution.hookData); } private <T> void runErrorHooks(List<HookExecution<T>> reversedHooks, Exception error, ...) { for (HookExecution<T> execution : reversedHooks) { try { execution.hook.error(contextFor(execution, ...), error, hookHints); } catch (Exception e) { log.error("error executing provider hook error stage", e); } } } private <T> void runFinallyHooks(List<HookExecution<T>> reversedHooks, FlagEvaluationDetails<T> details, ...) { ... } private <T> EvaluationContext runBeforeHooks(List<HookExecution<T>> hooks, EvaluationContext evaluatedContext, ...) { ... }
evaluatethen reduces to the before / provider call / after-or-error / finally sequence. Consider groupingkey,valueType,defaultValue,provider,hookExecutionContext, andhookHintsinto a small private request record to keep the parameter lists short.As per static analysis hints: "Refactor this method to reduce its Cognitive Complexity from 29 to the 15 allowed" and "Extract this nested try block into a separate method".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java` around lines 43 - 181, Refactor evaluate to reduce its cognitive complexity below the SonarCloud limit by extracting a context factory for HookExecution and separate helpers for before, error, and finally hook stages, while preserving registration/reverse execution order and behavior. Move the nested error-stage try/catch into the error-hook helper, and reuse the context factory instead of repeating createHookContext calls; optionally group shared parameters in a private request record.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`:
- Around line 94-175: Reduce cognitive complexity in ComparisonStrategy.evaluate
by extracting provider task creation and future-result collection into private
helper methods. Keep the existing provider iteration order, timeout handling,
cancellation behavior, Future.get propagation, and interruption restoration
unchanged; evaluate should continue delegating to those helpers while preserving
current errorResult handling.
- Around line 115-147: Update the task/future tracking in the comparison
strategy around providerFunction and invokeAll so each Future is associated with
its provider name. Before returning from the future.isCancelled() branch, add a
ProviderError for every cancelled provider to providerErrors, then call
errorResult as before. Add a regression test that blocks a provider, triggers
the timeout, and verifies the timed-out provider appears in the aggregate
providerErrors.
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java`:
- Around line 392-399: Update registerChildProviderObserver to remove any
previously registered observer for providerName from the child EventProvider
before creating and registering the new observer. Reuse the existing
providerEventObservers entry and unregisterChildProviderObserver behavior where
appropriate, then store only the newly registered observer so repeated
initialize calls do not accumulate listeners.
---
Nitpick comments:
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java`:
- Around line 264-352: Extract the duplicated strategy and hook-executor wiring
from the five evaluation methods into one generic helper in MultiProvider,
parameterized by value type, FlagValueType, and the terminal provider evaluation
BiFunction. Update getBooleanEvaluation, getStringEvaluation,
getIntegerEvaluation, getDoubleEvaluation, and getObjectEvaluation to delegate
to that helper, and add the required BiFunction import.
In
`@src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java`:
- Around line 43-181: Refactor evaluate to reduce its cognitive complexity below
the SonarCloud limit by extracting a context factory for HookExecution and
separate helpers for before, error, and finally hook stages, while preserving
registration/reverse execution order and behavior. Move the nested error-stage
try/catch into the error-hook helper, and reuse the context factory instead of
repeating createHookContext calls; optionally group shared parameters in a
private request record.
In
`@src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java`:
- Around line 78-113: Update shouldPreserveChildStateEmittedDuringInitialize and
shouldForwardTrackToReadyProvidersAndSkipFatalProviders to wrap initialization
and assertions in finally blocks that call multiProvider.shutdown(). Preserve
the existing test assertions while ensuring cleanup runs even when a test fails.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 998827c6-1f7a-4c57-95cb-f9153924ce5a
📒 Files selected for processing (10)
src/main/java/dev/openfeature/sdk/EventProvider.javasrc/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.javasrc/main/java/dev/openfeature/sdk/multiprovider/HookExecutionContext.javasrc/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.javasrc/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.javasrc/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutorTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java
| public <T> ProviderEvaluation<T> evaluate( | ||
| Map<String, FeatureProvider> providers, | ||
| String key, | ||
| T defaultValue, | ||
| EvaluationContext ctx, | ||
| Function<FeatureProvider, ProviderEvaluation<T>> providerFunction) { | ||
| if (providers.isEmpty()) { | ||
| return ProviderEvaluation.<T>builder() | ||
| .errorCode(ErrorCode.GENERAL) | ||
| .errorMessage("No providers configured") | ||
| .build(); | ||
| } | ||
| if (!providers.containsKey(fallbackProvider)) { | ||
| throw new IllegalArgumentException("fallbackProvider not found in providers: " + fallbackProvider); | ||
| } | ||
|
|
||
| int capacity = providers.size() * 4 / 3 + 1; | ||
| Map<String, ProviderEvaluation<T>> successfulResults = new ConcurrentHashMap<>(capacity); | ||
| Map<String, ProviderError> providerErrors = new ConcurrentHashMap<>(capacity); | ||
|
|
||
| try { | ||
| List<Callable<Void>> tasks = new ArrayList<>(providers.size()); | ||
| for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) { | ||
| String providerName = entry.getKey(); | ||
| FeatureProvider provider = entry.getValue(); | ||
| tasks.add(() -> { | ||
| try { | ||
| ProviderEvaluation<T> evaluation = providerFunction.apply(provider); | ||
| if (evaluation == null) { | ||
| providerErrors.put( | ||
| providerName, | ||
| ProviderError.fromResult(providerName, ErrorCode.GENERAL, "null evaluation")); | ||
| } else if (evaluation.getErrorCode() == null) { | ||
| successfulResults.put(providerName, evaluation); | ||
| } else { | ||
| providerErrors.put( | ||
| providerName, | ||
| ProviderError.fromResult( | ||
| providerName, evaluation.getErrorCode(), evaluation.getErrorMessage())); | ||
| } | ||
| } catch (Exception e) { | ||
| providerErrors.put(providerName, ProviderError.fromException(providerName, e)); | ||
| } | ||
| return null; | ||
| }); | ||
| } | ||
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | ||
| for (Future<Void> future : futures) { | ||
| if (future.isCancelled()) { | ||
| return errorResult( | ||
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors); | ||
| } | ||
| future.get(); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return errorResult("Comparison strategy interrupted: " + e.getMessage(), providers, providerErrors); | ||
| } catch (Exception e) { | ||
| return errorResult("Comparison strategy failed: " + e.getMessage(), providers, providerErrors); | ||
| } | ||
|
|
||
| if (!providerErrors.isEmpty()) { | ||
| return errorResult("Provider errors during comparison", providers, providerErrors); | ||
| } | ||
|
|
||
| ProviderEvaluation<T> fallbackResult = successfulResults.get(fallbackProvider); | ||
| if (fallbackResult == null) { | ||
| return errorResult( | ||
| "Fallback provider did not return a successful evaluation: " + fallbackProvider, | ||
| providers, | ||
| providerErrors); | ||
| } | ||
|
|
||
| if (allEvaluationsMatch(successfulResults)) { | ||
| return fallbackResult; | ||
| } | ||
|
|
||
| if (onMismatch != null) { | ||
| onMismatch.accept(key, orderedResults(providers, successfulResults)); | ||
| } | ||
| return fallbackResult; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reduce evaluate cognitive complexity to pass the configured check.
SonarCloud reports complexity 20 at Line 94. The configured limit is 15. Extract provider task creation and future-result collection into private methods. Preserve the current ordering, timeout, and interruption behavior.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 94-94: Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`
around lines 94 - 175, Reduce cognitive complexity in
ComparisonStrategy.evaluate by extracting provider task creation and
future-result collection into private helper methods. Keep the existing provider
iteration order, timeout handling, cancellation behavior, Future.get
propagation, and interruption restoration unchanged; evaluate should continue
delegating to those helpers while preserving current errorResult handling.
Source: Linters/SAST tools
| List<Callable<Void>> tasks = new ArrayList<>(providers.size()); | ||
| for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) { | ||
| String providerName = entry.getKey(); | ||
| FeatureProvider provider = entry.getValue(); | ||
| tasks.add(() -> { | ||
| try { | ||
| ProviderEvaluation<T> evaluation = providerFunction.apply(provider); | ||
| if (evaluation == null) { | ||
| providerErrors.put( | ||
| providerName, | ||
| ProviderError.fromResult(providerName, ErrorCode.GENERAL, "null evaluation")); | ||
| } else if (evaluation.getErrorCode() == null) { | ||
| successfulResults.put(providerName, evaluation); | ||
| } else { | ||
| providerErrors.put( | ||
| providerName, | ||
| ProviderError.fromResult( | ||
| providerName, evaluation.getErrorCode(), evaluation.getErrorMessage())); | ||
| } | ||
| } catch (Exception e) { | ||
| providerErrors.put(providerName, ProviderError.fromException(providerName, e)); | ||
| } | ||
| return null; | ||
| }); | ||
| } | ||
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | ||
| for (Future<Void> future : futures) { | ||
| if (future.isCancelled()) { | ||
| return errorResult( | ||
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors); | ||
| } | ||
| future.get(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record each timed-out provider before returning the aggregate error.
At Line 140, invokeAll cancels unfinished tasks. Lines 142-145 return an error before adding their provider names to providerErrors. A timeout can therefore produce an empty MultiProviderEvaluation.providerErrors list.
Track each future with its provider name. Add a ProviderError for every cancelled future before calling errorResult. Add a regression test that blocks a provider and asserts its timeout error is present.
Proposed fix
List<Callable<Void>> tasks = new ArrayList<>(providers.size());
+List<String> providerNames = new ArrayList<>(providers.size());
for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) {
String providerName = entry.getKey();
FeatureProvider provider = entry.getValue();
+ providerNames.add(providerName);
tasks.add(() -> {
// ...
});
}
List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS);
-for (Future<Void> future : futures) {
+boolean timedOut = false;
+for (int index = 0; index < futures.size(); index++) {
+ Future<Void> future = futures.get(index);
if (future.isCancelled()) {
- return errorResult(
- "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors);
+ String providerName = providerNames.get(index);
+ providerErrors.putIfAbsent(
+ providerName,
+ ProviderError.fromResult(
+ providerName,
+ ErrorCode.GENERAL,
+ "timed out after " + timeoutMs + "ms"));
+ timedOut = true;
+ continue;
}
future.get();
}
+if (timedOut) {
+ return errorResult(
+ "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| List<Callable<Void>> tasks = new ArrayList<>(providers.size()); | |
| for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) { | |
| String providerName = entry.getKey(); | |
| FeatureProvider provider = entry.getValue(); | |
| tasks.add(() -> { | |
| try { | |
| ProviderEvaluation<T> evaluation = providerFunction.apply(provider); | |
| if (evaluation == null) { | |
| providerErrors.put( | |
| providerName, | |
| ProviderError.fromResult(providerName, ErrorCode.GENERAL, "null evaluation")); | |
| } else if (evaluation.getErrorCode() == null) { | |
| successfulResults.put(providerName, evaluation); | |
| } else { | |
| providerErrors.put( | |
| providerName, | |
| ProviderError.fromResult( | |
| providerName, evaluation.getErrorCode(), evaluation.getErrorMessage())); | |
| } | |
| } catch (Exception e) { | |
| providerErrors.put(providerName, ProviderError.fromException(providerName, e)); | |
| } | |
| return null; | |
| }); | |
| } | |
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | |
| for (Future<Void> future : futures) { | |
| if (future.isCancelled()) { | |
| return errorResult( | |
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors); | |
| } | |
| future.get(); | |
| } | |
| List<Callable<Void>> tasks = new ArrayList<>(providers.size()); | |
| List<String> providerNames = new ArrayList<>(providers.size()); | |
| for (Map.Entry<String, FeatureProvider> entry : providers.entrySet()) { | |
| String providerName = entry.getKey(); | |
| FeatureProvider provider = entry.getValue(); | |
| providerNames.add(providerName); | |
| tasks.add(() -> { | |
| try { | |
| ProviderEvaluation<T> evaluation = providerFunction.apply(provider); | |
| if (evaluation == null) { | |
| providerErrors.put( | |
| providerName, | |
| ProviderError.fromResult(providerName, ErrorCode.GENERAL, "null evaluation")); | |
| } else if (evaluation.getErrorCode() == null) { | |
| successfulResults.put(providerName, evaluation); | |
| } else { | |
| providerErrors.put( | |
| providerName, | |
| ProviderError.fromResult( | |
| providerName, evaluation.getErrorCode(), evaluation.getErrorMessage())); | |
| } | |
| } catch (Exception e) { | |
| providerErrors.put(providerName, ProviderError.fromException(providerName, e)); | |
| } | |
| return null; | |
| }); | |
| } | |
| List<Future<Void>> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); | |
| boolean timedOut = false; | |
| for (int index = 0; index < futures.size(); index++) { | |
| Future<Void> future = futures.get(index); | |
| if (future.isCancelled()) { | |
| String providerName = providerNames.get(index); | |
| providerErrors.putIfAbsent( | |
| providerName, | |
| ProviderError.fromResult( | |
| providerName, | |
| ErrorCode.GENERAL, | |
| "timed out after " + timeoutMs + "ms")); | |
| timedOut = true; | |
| continue; | |
| } | |
| future.get(); | |
| } | |
| if (timedOut) { | |
| return errorResult( | |
| "Comparison strategy timed out after " + timeoutMs + "ms", providers, providerErrors); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java`
around lines 115 - 147, Update the task/future tracking in the comparison
strategy around providerFunction and invokeAll so each Future is associated with
its provider name. Before returning from the future.isCancelled() branch, add a
ProviderError for every cancelled provider to providerErrors, then call
errorResult as before. Add a regression test that blocks a provider, triggers
the timeout, and verifies the timed-out provider appears in the aggregate
providerErrors.
| private void registerChildProviderObserver(String providerName, FeatureProvider provider) { | ||
| if (provider instanceof EventProvider) { | ||
| BiConsumer<ProviderEvent, ProviderEventDetails> observer = | ||
| (event, details) -> onChildProviderEvent(providerName, event, details); | ||
| ((EventProvider) provider).addEventObserver(observer); | ||
| providerEventObservers.put(providerName, observer); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unregister the previous observer before you register a new one.
registerChildProviderObserver creates a new lambda and calls providerEventObservers.put(...). put replaces the map entry, but the earlier lambda stays registered on the child EventProvider. unregisterChildProviderObserver can then remove only the last observer.
initialize(...) can run more than once on the same MultiProvider instance, for example after shutdown(). Each extra run adds one more observer per child provider. The accumulated observers leak, and each duplicate re-forwards PROVIDER_CONFIGURATION_CHANGED, so listeners receive duplicate configuration-change events.
🐛 Proposed fix
private void registerChildProviderObserver(String providerName, FeatureProvider provider) {
if (provider instanceof EventProvider) {
+ // Drop any observer left over from a previous initialize() on this instance.
+ unregisterChildProviderObserver(providerName, provider);
BiConsumer<ProviderEvent, ProviderEventDetails> observer =
(event, details) -> onChildProviderEvent(providerName, event, details);
((EventProvider) provider).addEventObserver(observer);
providerEventObservers.put(providerName, observer);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void registerChildProviderObserver(String providerName, FeatureProvider provider) { | |
| if (provider instanceof EventProvider) { | |
| BiConsumer<ProviderEvent, ProviderEventDetails> observer = | |
| (event, details) -> onChildProviderEvent(providerName, event, details); | |
| ((EventProvider) provider).addEventObserver(observer); | |
| providerEventObservers.put(providerName, observer); | |
| } | |
| } | |
| private void registerChildProviderObserver(String providerName, FeatureProvider provider) { | |
| if (provider instanceof EventProvider) { | |
| // Drop any observer left over from a previous initialize() on this instance. | |
| unregisterChildProviderObserver(providerName, provider); | |
| BiConsumer<ProviderEvent, ProviderEventDetails> observer = | |
| (event, details) -> onChildProviderEvent(providerName, event, details); | |
| ((EventProvider) provider).addEventObserver(observer); | |
| providerEventObservers.put(providerName, observer); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java` around
lines 392 - 399, Update registerChildProviderObserver to remove any previously
registered observer for providerName from the child EventProvider before
creating and registering the new observer. Reuse the existing
providerEventObservers entry and unregisterChildProviderObserver behavior where
appropriate, then store only the newly registered observer so repeated
initialize calls do not accumulate listeners.
Summary
OpenFeatureClientfromMultiProviderfor hook context capture, uses a provider-levelbeforehook with aThreadLocal, matching the JS SDK'sWeakMappatterntrack()delegation toMultiProviderComparisonStrategy, parallel evaluation across all providers with configurable fallback and mismatch callbacktrack()forwardingImplementation
MultiProvider.getProviderHooks()returns abeforehook that capturesClientMetadataand a defensive copy of the hook hints into aThreadLocal; afinallyAfterhook cleans it up. Removes theOpenFeatureClient→MultiProviderimport dependency.ProviderStatemap with severity-based aggregate state (FATAL > NOT_READY > ERROR > STALE > READY). Registers/deregistersEventProviderobservers to react to child provider state changes.PROVIDER_CONFIGURATION_CHANGEDis always forwarded.MultiProviderHookExecutorruns each child provider's own hooks around its evaluation:beforein registration order,after/error/finallyAfterin reverse, with an isolated context copy per provider.ComparisonStrategyForkJoinPool.commonPool()(or a caller-supplied executor). Returns the fallback provider's result on agreement; invokes the optionalonMismatchcallback on disagreement. On failure returns aMultiProviderEvaluationcarrying per-providerProviderErrordetails, consistent withFirstMatchStrategy/FirstSuccessfulStrategy.track()NOT_READY/FATALones. Per-provider errors are logged, not propagated.buildProvidersnow appends-1,-2suffixes instead of silently dropping duplicates.EventProvideraddEventObserver/removeEventObserverfor composite provider patterns.Notes
initialize(ctx, domain)from feat: supply bound domain to provider initialization #1982 (forwarded to child providers) andOpenFeatureAPI.createIsolated()from feat: support isolated API instances #1928 (used by the new tests).MultiProvideronly acceptsList<FeatureProvider>with no explicit-naming API, so there's nothing to conflict with; only the metadata-derived auto-dedup applies here.Related Issues
Fixes #1882
Replaces #1883