From a1443003a7b6adac212e76b775d6523014b867e5 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Thu, 8 Oct 2026 11:37:22 -0400 Subject: [PATCH] feat: avoid instantiating un-watched errors * synthetic errors created for the error hook method are only created lazily, if such error hooks exist Signed-off-by: Todd Baert --- .../java/dev/openfeature/sdk/HookSupport.java | 51 ++++++++++++++++--- .../dev/openfeature/sdk/HookSupportData.java | 1 + .../sdk/MultiProviderHookExecutor.java | 9 ++-- .../openfeature/sdk/OpenFeatureClient.java | 50 +++++++++++++++--- .../dev/openfeature/sdk/HookSupportTest.java | 34 ++++++++++++- .../sdk/OpenFeatureClientTest.java | 23 +++++++++ .../sdk/fixtures/HookFixtures.java | 39 +++++++++++--- 7 files changed, 180 insertions(+), 27 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index 0891134b3..cb435c714 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -3,7 +3,9 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.function.Supplier; import lombok.extern.slf4j.Slf4j; /** @@ -13,6 +15,21 @@ @Slf4j class HookSupport { + private static final ClassValue IMPLEMENTS_ERROR_STAGE = new ClassValue<>() { + @Override + protected Boolean computeValue(Class hookClass) { + try { + return hookClass + .getMethod("error", HookContext.class, Exception.class, Map.class) + .getDeclaringClass() + != Hook.class; + } catch (NoSuchMethodException e) { + // unexpected; assume the error stage is implemented because this should never happen + return true; + } + } + }; + /** * Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object with {@link HookContext} * set to null. Filters hooks by supported {@link FlagValueType}. @@ -36,23 +53,32 @@ public void setHooks( Collection apiHooks, FlagValueType type) { List> hookContextPairs = new ArrayList<>(); - addFilteredHooks(hookContextPairs, providerHooks, type); - addFilteredHooks(hookContextPairs, optionHooks, type); - addFilteredHooks(hookContextPairs, clientHooks, type); - addFilteredHooks(hookContextPairs, apiHooks, type); + boolean hasErrorHooks = addFilteredHooks(hookContextPairs, providerHooks, type); + hasErrorHooks |= addFilteredHooks(hookContextPairs, optionHooks, type); + hasErrorHooks |= addFilteredHooks(hookContextPairs, clientHooks, type); + hasErrorHooks |= addFilteredHooks(hookContextPairs, apiHooks, type); hookSupportData.hooks = hookContextPairs; + hookSupportData.hasErrorHooks = hasErrorHooks; } - private static void addFilteredHooks( + /** + * Adds the hooks supporting the given type to dest. + * + * @return true if any added hook implements the error stage + */ + private static boolean addFilteredHooks( List> dest, Collection source, FlagValueType type) { if (source.isEmpty()) { - return; + return false; } + boolean hasErrorHooks = false; for (Hook hook : source) { if (hook.supportsFlagValueType(type)) { dest.add(Pair.of(hook, null)); + hasErrorHooks |= IMPLEMENTS_ERROR_STAGE.get(hook.getClass()); } } + return hasErrorHooks; } /** @@ -94,7 +120,18 @@ public void executeBeforeHooks(HookSupportData data) { } } - public void executeErrorHooks(HookSupportData data, Exception error) { + /** + * Runs the error stage of all hooks. The error is only created if a hook implements the error stage, + * avoiding the cost of instantiating (and potentially capturing a stack trace for) unused exceptions. + * + * @param data the hook support data + * @param errorSupplier supplies the error passed to the hooks + */ + public void executeErrorHooks(HookSupportData data, Supplier errorSupplier) { + if (!data.hasErrorHooks) { + return; + } + Exception error = errorSupplier.get(); for (Pair hookContextPair : data.getHooks()) { var hook = hookContextPair.getKey(); var hookContext = hookContextPair.getValue(); diff --git a/src/main/java/dev/openfeature/sdk/HookSupportData.java b/src/main/java/dev/openfeature/sdk/HookSupportData.java index 174702ea2..2a1128e21 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupportData.java +++ b/src/main/java/dev/openfeature/sdk/HookSupportData.java @@ -11,6 +11,7 @@ class HookSupportData { List> hooks; + boolean hasErrorHooks; LayeredEvaluationContext evaluationContext; Map hints; diff --git a/src/main/java/dev/openfeature/sdk/MultiProviderHookExecutor.java b/src/main/java/dev/openfeature/sdk/MultiProviderHookExecutor.java index bf5b4da2f..b683c6891 100644 --- a/src/main/java/dev/openfeature/sdk/MultiProviderHookExecutor.java +++ b/src/main/java/dev/openfeature/sdk/MultiProviderHookExecutor.java @@ -61,10 +61,11 @@ public ProviderEvaluation execute( ProviderEvaluation providerEvaluation = providerFunction.apply(provider, data.getEvaluationContext()); details = FlagEvaluationDetails.from(providerEvaluation, key); if (details.getErrorCode() != null) { - Exception error = - ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); + var errorCode = details.getErrorCode(); + var errorMessage = details.getErrorMessage(); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.executeErrorHooks(data, error); + hookSupport.executeErrorHooks( + data, () -> ExceptionUtils.instantiateErrorByErrorCode(errorCode, errorMessage)); } else { hookSupport.executeAfterHooks(data, details); } @@ -80,7 +81,7 @@ public ProviderEvaluation execute( } details.setErrorMessage(e.getMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.executeErrorHooks(data, e); + hookSupport.executeErrorHooks(data, () -> e); throw e; } finally { // details is always set by now: from the evaluation on success, or the catch on failure diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 66caf76e5..9364d892f 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -16,6 +16,7 @@ import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import java.util.function.Supplier; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -38,6 +39,9 @@ @Deprecated() // TODO: eventually we will make this non-public. See issue #872 public class OpenFeatureClient implements Client { + private static final String PROVIDER_NOT_READY_MESSAGE = "Provider not yet initialized"; + private static final String PROVIDER_FATAL_MESSAGE = "Provider is in an irrecoverable error state"; + private final OpenFeatureAPI openfeatureApi; @Getter @@ -201,12 +205,26 @@ private FlagEvaluationDetails evaluateFlag( hookSupport.executeBeforeHooks(hookSupportData); - // "short circuit" if the provider is in NOT_READY or FATAL state + // "short circuit" if the provider is in NOT_READY or FATAL state, without throwing if (ProviderState.NOT_READY.equals(state)) { - throw new ProviderNotReadyError("Provider not yet initialized"); + details = shortCircuit( + hookSupportData, + key, + defaultValue, + ErrorCode.PROVIDER_NOT_READY, + PROVIDER_NOT_READY_MESSAGE, + () -> new ProviderNotReadyError(PROVIDER_NOT_READY_MESSAGE)); + return details; } if (ProviderState.FATAL.equals(state)) { - throw new FatalError("Provider is in an irrecoverable error state"); + details = shortCircuit( + hookSupportData, + key, + defaultValue, + ErrorCode.PROVIDER_FATAL, + PROVIDER_FATAL_MESSAGE, + () -> new FatalError(PROVIDER_FATAL_MESSAGE)); + return details; } var providerEval = (ProviderEvaluation) @@ -214,10 +232,11 @@ private FlagEvaluationDetails evaluateFlag( details = FlagEvaluationDetails.from(providerEval, key); if (details.getErrorCode() != null) { - var error = - ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); + var errorCode = details.getErrorCode(); + var errorMessage = details.getErrorMessage(); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.executeErrorHooks(hookSupportData, error); + hookSupport.executeErrorHooks( + hookSupportData, () -> ExceptionUtils.instantiateErrorByErrorCode(errorCode, errorMessage)); } else { hookSupport.executeAfterHooks(hookSupportData, details); } @@ -233,7 +252,7 @@ private FlagEvaluationDetails evaluateFlag( details.setErrorMessage(e.getMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); if (hookSupportData.getHooks() != null) { - hookSupport.executeErrorHooks(hookSupportData, e); + hookSupport.executeErrorHooks(hookSupportData, () -> e); } } finally { if (hookSupportData.getHooks() != null) { @@ -244,6 +263,23 @@ private FlagEvaluationDetails evaluateFlag( return details; } + private FlagEvaluationDetails shortCircuit( + HookSupportData hookSupportData, + String key, + T defaultValue, + ErrorCode errorCode, + String errorMessage, + Supplier errorSupplier) { + FlagEvaluationDetails details = FlagEvaluationDetails.builder() + .flagKey(key) + .errorCode(errorCode) + .errorMessage(errorMessage) + .build(); + enrichDetailsWithErrorDefaults(defaultValue, details); + hookSupport.executeErrorHooks(hookSupportData, errorSupplier); + return details; + } + private static void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails details) { details.setValue(defaultValue); details.setReason(Reason.ERROR.toString()); diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index c2b7831c5..f1920b9ed 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -15,10 +15,12 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.ValueSource; class HookSupportTest implements HookFixtures { @@ -110,7 +112,7 @@ void shouldPassDataAcrossStages(FlagValueType flagValueType) { hookSupportData, FlagEvaluationDetails.builder().build()); assertHookData(testHook, "before", "after", "finallyAfter"); - hookSupport.executeErrorHooks(hookSupportData, mock(Exception.class)); + hookSupport.executeErrorHooks(hookSupportData, () -> mock(Exception.class)); assertHookData(testHook, "before", "after", "finallyAfter", "error"); } @@ -232,7 +234,7 @@ private static void callAllHooks(HookSupportData hookSupportData) { hookSupportData, FlagEvaluationDetails.builder().build()); hookSupport.executeAfterAllHooks( hookSupportData, FlagEvaluationDetails.builder().build()); - hookSupport.executeErrorHooks(hookSupportData, mock(Exception.class)); + hookSupport.executeErrorHooks(hookSupportData, () -> mock(Exception.class)); } private static void assertHookData(TestHookWithData testHook, String... expectedKeys) { @@ -285,4 +287,32 @@ private EvaluationContext evaluationContextWithValue(String key, String value) { attributes.put(key, new Value(value)); return new ImmutableContext(attributes); } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @DisplayName("should only create the error if a hook implements the error stage") + void shouldOnlyCreateErrorIfAHookImplementsErrorStage(boolean implementsErrorStage) { + Hook hook = implementsErrorStage ? mockBooleanHook() : new BooleanHook() {}; + var hookSupportData = new HookSupportData(); + hookSupport.setHooks( + hookSupportData, + List.of(hook), + Collections.emptyList(), + Collections.emptyList(), + Collections.emptyList(), + FlagValueType.BOOLEAN); + hookSupport.setHookContexts( + hookSupportData, + getBaseHookContextForType(FlagValueType.BOOLEAN), + new LayeredEvaluationContext(null, null, null, null)); + + var errorsCreated = new AtomicInteger(); + hookSupport.executeErrorHooks(hookSupportData, () -> { + errorsCreated.incrementAndGet(); + return new Exception(); + }); + + // created once if a hook can receive it, otherwise never + assertThat(errorsCreated).hasValue(implementsErrorStage ? 1 : 0); + } } diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java index 31937ec2d..8ce8ff4b3 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java @@ -9,6 +9,7 @@ import static org.mockito.Mockito.never; import dev.openfeature.sdk.exceptions.FatalError; +import dev.openfeature.sdk.exceptions.ProviderNotReadyError; import dev.openfeature.sdk.fixtures.HookFixtures; import dev.openfeature.sdk.testutils.testProvider.TestProvider; import java.util.HashMap; @@ -186,4 +187,26 @@ public Optional before(HookContext ctx, Map details = + client.getBooleanDetails("key", true, new ImmutableContext(), options); + + assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY); + assertThat(details.getValue()).isTrue(); + // created once if a hook can receive it, otherwise never + assertThat(errors.constructed()).hasSize(withErrorHook ? 1 : 0); + } + } } diff --git a/src/test/java/dev/openfeature/sdk/fixtures/HookFixtures.java b/src/test/java/dev/openfeature/sdk/fixtures/HookFixtures.java index a240af991..4b85dc6ec 100644 --- a/src/test/java/dev/openfeature/sdk/fixtures/HookFixtures.java +++ b/src/test/java/dev/openfeature/sdk/fixtures/HookFixtures.java @@ -5,38 +5,63 @@ import dev.openfeature.sdk.BooleanHook; import dev.openfeature.sdk.DoubleHook; import dev.openfeature.sdk.Hook; +import dev.openfeature.sdk.HookContext; import dev.openfeature.sdk.IntegerHook; import dev.openfeature.sdk.LongHook; import dev.openfeature.sdk.ObjectHook; import dev.openfeature.sdk.StringHook; +import java.util.Map; public interface HookFixtures { + // the SDK only invokes the error stage of hooks that implement it, so these fixtures override it + default Hook mockBooleanHook() { - return spy(BooleanHook.class); + return spy(new BooleanHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockStringHook() { - return spy(StringHook.class); + return spy(new StringHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockIntegerHook() { - return spy(IntegerHook.class); + return spy(new IntegerHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockLongHook() { - return spy(LongHook.class); + return spy(new LongHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockDoubleHook() { - return spy(DoubleHook.class); + return spy(new DoubleHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockObjectHook() { - return spy(ObjectHook.class); + return spy(new ObjectHook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } default Hook mockGenericHook() { - return spy(Hook.class); + return spy(new Hook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) {} + }); } }