From 21929328630eba00be741392457f68bacf59f376 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Tue, 11 Oct 2022 18:43:50 -0400 Subject: [PATCH] feat!: add rw locks to client/api, hook accessor name (#131) * fix: add read/write locks to client/api Signed-off-by: Todd Baert * dont lock entire evaluation Signed-off-by: Todd Baert * add tests Signed-off-by: Todd Baert * fixup comment Signed-off-by: Todd Baert * fixup pom comment Signed-off-by: Todd Baert * increase lock granularity, imporove tests Signed-off-by: Todd Baert * fix spotbugs Signed-off-by: Todd Baert * remove commented test Signed-off-by: Todd Baert Signed-off-by: Todd Baert --- pom.xml | 18 +++ spotbugs-exclusions.xml | 17 ++- src/main/java/dev/openfeature/sdk/Client.java | 2 +- .../dev/openfeature/sdk/OpenFeatureAPI.java | 91 +++++++++--- .../openfeature/sdk/OpenFeatureClient.java | 77 +++++++--- .../sdk/internal/AutoCloseableLock.java | 10 ++ .../AutoCloseableReentrantReadWriteLock.java | 28 ++++ .../sdk/DeveloperExperienceTest.java | 36 ++++- .../openfeature/sdk/DoSomethingProvider.java | 3 +- .../sdk/FlagEvaluationSpecTest.java | 12 +- .../java/dev/openfeature/sdk/LockingTest.java | 132 ++++++++++++++++++ .../sdk/OpenFeatureClientTest.java | 2 +- 12 files changed, 379 insertions(+), 49 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/internal/AutoCloseableLock.java create mode 100644 src/main/java/dev/openfeature/sdk/internal/AutoCloseableReentrantReadWriteLock.java create mode 100644 src/test/java/dev/openfeature/sdk/LockingTest.java diff --git a/pom.xml b/pom.xml index e15949da..443da5ce 100644 --- a/pom.xml +++ b/pom.xml @@ -164,6 +164,21 @@ + + org.codehaus.mojo + build-helper-maven-plugin + 3.3.0 + + + validate + get-cpu-count + + cpu-count + + + + + org.cyclonedx cyclonedx-maven-plugin @@ -231,6 +246,9 @@ ${surefireArgLine} + + ${cpu.count} + false ${testExclusions} diff --git a/spotbugs-exclusions.xml b/spotbugs-exclusions.xml index 8675964d..673bf4b5 100644 --- a/spotbugs-exclusions.xml +++ b/spotbugs-exclusions.xml @@ -9,11 +9,26 @@ - + + + + + + + + + + + + + + + + diff --git a/src/main/java/dev/openfeature/sdk/Client.java b/src/main/java/dev/openfeature/sdk/Client.java index 07015a63..a4ccf26f 100644 --- a/src/main/java/dev/openfeature/sdk/Client.java +++ b/src/main/java/dev/openfeature/sdk/Client.java @@ -32,5 +32,5 @@ public interface Client extends Features { * Fetch the hooks associated to this client. * @return A list of {@link Hook}s. */ - List getClientHooks(); + List getHooks(); } diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index 5918fa08..e85f4e13 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -1,43 +1,41 @@ package dev.openfeature.sdk; -import lombok.Getter; -import lombok.Setter; - -import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import javax.annotation.Nullable; + +import dev.openfeature.sdk.internal.AutoCloseableLock; +import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; + /** * A global singleton which holds base configuration for the OpenFeature library. * Configuration here will be shared across all {@link Client}s. */ public class OpenFeatureAPI { - private static OpenFeatureAPI api; - @Getter - @Setter + // package-private multi-read/single-write lock + static AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock(); + static AutoCloseableReentrantReadWriteLock providerLock = new AutoCloseableReentrantReadWriteLock(); + static AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock(); private FeatureProvider provider; - @Getter - @Setter private EvaluationContext evaluationContext; - @Getter private List apiHooks; - public OpenFeatureAPI() { + private OpenFeatureAPI() { this.apiHooks = new ArrayList<>(); } + private static class SingletonHolder { + private static final OpenFeatureAPI INSTANCE = new OpenFeatureAPI(); + } + /** * Provisions the {@link OpenFeatureAPI} singleton (if needed) and returns it. * @return The singleton instance. */ public static OpenFeatureAPI getInstance() { - synchronized (OpenFeatureAPI.class) { - if (api == null) { - api = new OpenFeatureAPI(); - } - } - return api; + return SingletonHolder.INSTANCE; } public Metadata getProviderMetadata() { @@ -56,11 +54,66 @@ public Client getClient(@Nullable String name, @Nullable String version) { return new OpenFeatureClient(this, name, version); } + /** + * {@inheritDoc} + */ + public void setEvaluationContext(EvaluationContext evaluationContext) { + try (AutoCloseableLock __ = contextLock.writeLockAutoCloseable()) { + this.evaluationContext = evaluationContext; + } + } + + /** + * {@inheritDoc} + */ + public EvaluationContext getEvaluationContext() { + try (AutoCloseableLock __ = contextLock.readLockAutoCloseable()) { + return this.evaluationContext; + } + } + + /** + * {@inheritDoc} + */ + public void setProvider(FeatureProvider provider) { + try (AutoCloseableLock __ = providerLock.writeLockAutoCloseable()) { + this.provider = provider; + } + } + + /** + * {@inheritDoc} + */ + public FeatureProvider getProvider() { + try (AutoCloseableLock __ = providerLock.readLockAutoCloseable()) { + return this.provider; + } + } + + /** + * {@inheritDoc} + */ public void addHooks(Hook... hooks) { - this.apiHooks.addAll(Arrays.asList(hooks)); + try (AutoCloseableLock __ = hooksLock.writeLockAutoCloseable()) { + this.apiHooks.addAll(Arrays.asList(hooks)); + } } + /** + * {@inheritDoc} + */ + public List getHooks() { + try (AutoCloseableLock __ = hooksLock.readLockAutoCloseable()) { + return this.apiHooks; + } + } + + /** + * {@inheritDoc} + */ public void clearHooks() { - this.apiHooks.clear(); + try (AutoCloseableLock __ = hooksLock.writeLockAutoCloseable()) { + this.apiHooks.clear(); + } } } diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 0f99e494..3759ecea 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -8,9 +8,10 @@ import dev.openfeature.sdk.exceptions.GeneralError; import dev.openfeature.sdk.exceptions.OpenFeatureError; +import dev.openfeature.sdk.internal.AutoCloseableLock; +import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; import dev.openfeature.sdk.internal.ObjectUtils; import lombok.Getter; -import lombok.Setter; import lombok.extern.slf4j.Slf4j; @Slf4j @@ -22,12 +23,10 @@ public class OpenFeatureClient implements Client { private final String name; @Getter private final String version; - @Getter private final List clientHooks; private final HookSupport hookSupport; - - @Getter - @Setter + AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock(); + AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock(); private EvaluationContext evaluationContext; /** @@ -46,9 +45,44 @@ public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String name, String vers this.hookSupport = new HookSupport(); } + /** + * {@inheritDoc} + */ @Override public void addHooks(Hook... hooks) { - this.clientHooks.addAll(Arrays.asList(hooks)); + try (AutoCloseableLock __ = this.hooksLock.writeLockAutoCloseable()) { + this.clientHooks.addAll(Arrays.asList(hooks)); + } + } + + /** + * {@inheritDoc} + */ + @Override + public List getHooks() { + try (AutoCloseableLock __ = this.hooksLock.readLockAutoCloseable()) { + return this.clientHooks; + } + } + + /** + * {@inheritDoc} + */ + @Override + public void setEvaluationContext(EvaluationContext evaluationContext) { + try (AutoCloseableLock __ = contextLock.writeLockAutoCloseable()) { + this.evaluationContext = evaluationContext; + } + } + + /** + * {@inheritDoc} + */ + @Override + public EvaluationContext getEvaluationContext() { + try (AutoCloseableLock __ = contextLock.readLockAutoCloseable()) { + return this.evaluationContext; + } } private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key, T defaultValue, @@ -57,34 +91,41 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key () -> FlagEvaluationOptions.builder().build()); Map hints = Collections.unmodifiableMap(flagOptions.getHookHints()); ctx = ObjectUtils.defaultIfNull(ctx, () -> new MutableContext()); - FeatureProvider provider = ObjectUtils.defaultIfNull(openfeatureApi.getProvider(), () -> { - log.debug("No provider configured, using no-op provider."); - return new NoOpProvider(); - }); + FlagEvaluationDetails details = null; List mergedHooks = null; HookContext hookCtx = null; + FeatureProvider provider = null; try { + final EvaluationContext apiContext; + final EvaluationContext clientContext; - hookCtx = HookContext.from(key, type, this.getMetadata(), - openfeatureApi.getProvider().getMetadata(), ctx, defaultValue); + // openfeatureApi.getProvider() must be called once to maintain a consistent reference + provider = ObjectUtils.defaultIfNull(openfeatureApi.getProvider(), () -> { + log.debug("No provider configured, using no-op provider."); + return new NoOpProvider(); + }); mergedHooks = ObjectUtils.merge(provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, - openfeatureApi.getApiHooks()); + openfeatureApi.getHooks()); - EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints); - - EvaluationContext invocationCtx = ctx.merge(ctxFromHook); + hookCtx = HookContext.from(key, type, this.getMetadata(), + provider.getMetadata(), ctx, defaultValue); // merge of: API.context, client.context, invocation.context - EvaluationContext apiContext = openfeatureApi.getEvaluationContext() != null + apiContext = openfeatureApi.getEvaluationContext() != null ? openfeatureApi.getEvaluationContext() : new MutableContext(); - EvaluationContext clientContext = openfeatureApi.getEvaluationContext() != null + clientContext = openfeatureApi.getEvaluationContext() != null ? this.getEvaluationContext() : new MutableContext(); + + EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints); + + EvaluationContext invocationCtx = ctx.merge(ctxFromHook); + EvaluationContext mergedCtx = apiContext.merge(clientContext.merge(invocationCtx)); ProviderEvaluation providerEval = (ProviderEvaluation) createProviderEvaluation(type, key, diff --git a/src/main/java/dev/openfeature/sdk/internal/AutoCloseableLock.java b/src/main/java/dev/openfeature/sdk/internal/AutoCloseableLock.java new file mode 100644 index 00000000..41fb5dc9 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/internal/AutoCloseableLock.java @@ -0,0 +1,10 @@ +package dev.openfeature.sdk.internal; + +public interface AutoCloseableLock extends AutoCloseable { + + /** + * Override the exception in AutoClosable. + */ + @Override + void close(); +} diff --git a/src/main/java/dev/openfeature/sdk/internal/AutoCloseableReentrantReadWriteLock.java b/src/main/java/dev/openfeature/sdk/internal/AutoCloseableReentrantReadWriteLock.java new file mode 100644 index 00000000..92827ef6 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/internal/AutoCloseableReentrantReadWriteLock.java @@ -0,0 +1,28 @@ +package dev.openfeature.sdk.internal; + +import java.util.concurrent.locks.ReentrantReadWriteLock; + +/** + * A utility class that wraps a multi-read/single-write lock construct as AutoCloseable, so it can + * be used in a try-with-resources. + */ +public class AutoCloseableReentrantReadWriteLock extends ReentrantReadWriteLock { + + /** + * Get the single write lock as an AutoCloseableLock. + * @return unlock method ref + */ + public AutoCloseableLock writeLockAutoCloseable() { + this.writeLock().lock(); + return this.writeLock()::unlock; + } + + /** + * Get the multi read lock as an AutoCloseableLock. + * @return unlock method ref + */ + public AutoCloseableLock readLockAutoCloseable() { + this.readLock().lock(); + return this.readLock()::unlock; + } +} \ No newline at end of file diff --git a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java index 454f1670..12e03abe 100644 --- a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java +++ b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java @@ -6,12 +6,14 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import java.util.Arrays; +import java.util.Map; +import java.util.Optional; + import org.junit.jupiter.api.Test; import dev.openfeature.sdk.fixtures.HookFixtures; -import java.util.Arrays; - class DeveloperExperienceTest implements HookFixtures { transient String flagKey = "mykey"; @@ -90,4 +92,34 @@ class DeveloperExperienceTest implements HookFixtures { assertEquals(Reason.ERROR.toString(), retval.getReason()); assertFalse(retval.getValue()); } + + @Test + void providerLockedPerTransaction() throws InterruptedException { + + class MutatingHook implements Hook { + + @Override + // change the provider during a before hook - this should not impact the evaluation in progress + public Optional before(HookContext ctx, Map hints) { + OpenFeatureAPI.getInstance().setProvider(new NoOpProvider()); + return Optional.empty(); + } + } + + final String defaultValue = "string-value"; + final OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + final Client client = api.getClient(); + api.setProvider(new DoSomethingProvider()); + api.addHooks(new MutatingHook()); + + // if provider is changed during an evaluation transaction it should proceed with the original provider + String doSomethingValue = client.getStringValue("val", defaultValue); + assertEquals(new StringBuilder(defaultValue).reverse().toString(), doSomethingValue); + + api.clearHooks(); + + // subsequent evaluations should now use new provider set by hook + String noOpValue = client.getStringValue("val", defaultValue); + assertEquals(noOpValue, defaultValue); + } } diff --git a/src/test/java/dev/openfeature/sdk/DoSomethingProvider.java b/src/test/java/dev/openfeature/sdk/DoSomethingProvider.java index 37fd20f4..d87fa374 100644 --- a/src/test/java/dev/openfeature/sdk/DoSomethingProvider.java +++ b/src/test/java/dev/openfeature/sdk/DoSomethingProvider.java @@ -2,6 +2,7 @@ public class DoSomethingProvider implements FeatureProvider { + public static final String name = "Something"; private EvaluationContext savedContext; public EvaluationContext getMergedContext() { @@ -10,7 +11,7 @@ public EvaluationContext getMergedContext() { @Override public Metadata getMetadata() { - return () -> "test"; + return () -> name; } @Override diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java index 75b6e5bb..0bf9a6d4 100644 --- a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java @@ -53,7 +53,7 @@ private Client _client() { @Test void provider_metadata() { OpenFeatureAPI api = OpenFeatureAPI.getInstance(); api.setProvider(new DoSomethingProvider()); - assertEquals("test", api.getProviderMetadata().getName()); + assertEquals(DoSomethingProvider.name, api.getProviderMetadata().getName()); } @Specification(number="1.1.3", text="The API MUST provide a function to add hooks which accepts one or more API-conformant hooks, and appends them to the collection of any previously added hooks. When new hooks are added, previously added hooks are not removed.") @@ -63,12 +63,12 @@ private Client _client() { OpenFeatureAPI api = OpenFeatureAPI.getInstance(); api.addHooks(h1); - assertEquals(1, api.getApiHooks().size()); - assertEquals(h1, api.getApiHooks().get(0)); + assertEquals(1, api.getHooks().size()); + assertEquals(h1, api.getHooks().get(0)); api.addHooks(h2); - assertEquals(2, api.getApiHooks().size()); - assertEquals(h2, api.getApiHooks().get(1)); + assertEquals(2, api.getHooks().size()); + assertEquals(h2, api.getHooks().get(1)); } @Specification(number="1.1.5", text="The API MUST provide a function for creating a client which accepts the following options: - name (optional): A logical string identifier for the client.") @@ -85,7 +85,7 @@ private Client _client() { Hook m2 = mock(Hook.class); c.addHooks(m1); c.addHooks(m2); - List hooks = c.getClientHooks(); + List hooks = c.getHooks(); assertEquals(2, hooks.size()); assertTrue(hooks.contains(m1)); assertTrue(hooks.contains(m2)); diff --git a/src/test/java/dev/openfeature/sdk/LockingTest.java b/src/test/java/dev/openfeature/sdk/LockingTest.java new file mode 100644 index 00000000..1a8dac9c --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/LockingTest.java @@ -0,0 +1,132 @@ +package dev.openfeature.sdk; + +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.concurrent.locks.ReentrantReadWriteLock; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; + +class LockingTest { + + private static OpenFeatureAPI api; + private OpenFeatureClient client; + private AutoCloseableReentrantReadWriteLock apiContextLock; + private AutoCloseableReentrantReadWriteLock apiHooksLock; + private AutoCloseableReentrantReadWriteLock apiProviderLock; + private AutoCloseableReentrantReadWriteLock clientContextLock; + private AutoCloseableReentrantReadWriteLock clientHooksLock; + + @BeforeAll + static void beforeAll() { + api = OpenFeatureAPI.getInstance(); + } + + @BeforeEach + void beforeEach() { + client = (OpenFeatureClient) api.getClient(); + + apiContextLock = setupLock(apiContextLock, mockInnerReadLock(), mockInnerWriteLock()); + apiProviderLock = setupLock(apiProviderLock, mockInnerReadLock(), mockInnerWriteLock()); + apiHooksLock = setupLock(apiHooksLock, mockInnerReadLock(), mockInnerWriteLock()); + OpenFeatureAPI.contextLock = apiContextLock; + OpenFeatureAPI.providerLock = apiProviderLock; + OpenFeatureAPI.hooksLock = apiHooksLock; + + clientContextLock = setupLock(clientContextLock, mockInnerReadLock(), mockInnerWriteLock()); + clientHooksLock = setupLock(clientHooksLock, mockInnerReadLock(), mockInnerWriteLock()); + client.contextLock = clientContextLock; + client.hooksLock = clientHooksLock; + } + + @Test + void addHooksShouldWriteLockAndUnlock() { + client.addHooks(new Hook() { + }); + verify(clientHooksLock.writeLock()).lock(); + verify(clientHooksLock.writeLock()).unlock(); + + api.addHooks(new Hook() { + }); + verify(apiHooksLock.writeLock()).lock(); + verify(apiHooksLock.writeLock()).unlock(); + } + + @Test + void getHooksShouldReadLockAndUnlock() { + client.getHooks(); + verify(clientHooksLock.readLock()).lock(); + verify(clientHooksLock.readLock()).unlock(); + + api.getHooks(); + verify(apiHooksLock.readLock()).lock(); + verify(apiHooksLock.readLock()).unlock(); + } + + @Test + void setContextShouldWriteLockAndUnlock() { + client.setEvaluationContext(new MutableContext()); + verify(clientContextLock.writeLock()).lock(); + verify(clientContextLock.writeLock()).unlock(); + + api.setEvaluationContext(new MutableContext()); + verify(apiContextLock.writeLock()).lock(); + verify(apiContextLock.writeLock()).unlock(); + } + + @Test + void getContextShouldReadLockAndUnlock() { + client.getEvaluationContext(); + verify(clientContextLock.readLock()).lock(); + verify(clientContextLock.readLock()).unlock(); + + api.getEvaluationContext(); + verify(apiContextLock.readLock()).lock(); + verify(apiContextLock.readLock()).unlock(); + } + + @Test + void setProviderShouldWriteLockAndUnlock() { + api.setProvider(new DoSomethingProvider()); + verify(apiProviderLock.writeLock()).lock(); + verify(apiProviderLock.writeLock()).unlock(); + } + + @Test + void clearHooksShouldWriteLockAndUnlock() { + api.clearHooks(); + verify(apiHooksLock.writeLock()).lock(); + verify(apiHooksLock.writeLock()).unlock(); + } + + private static ReentrantReadWriteLock.ReadLock mockInnerReadLock() { + ReentrantReadWriteLock.ReadLock readLockMock = mock(ReentrantReadWriteLock.ReadLock.class); + doNothing().when(readLockMock).lock(); + doNothing().when(readLockMock).unlock(); + return readLockMock; + } + + private static ReentrantReadWriteLock.WriteLock mockInnerWriteLock() { + ReentrantReadWriteLock.WriteLock writeLockMock = mock(ReentrantReadWriteLock.WriteLock.class); + doNothing().when(writeLockMock).lock(); + doNothing().when(writeLockMock).unlock(); + return writeLockMock; + } + + private AutoCloseableReentrantReadWriteLock setupLock(AutoCloseableReentrantReadWriteLock lock, + AutoCloseableReentrantReadWriteLock.ReadLock readlock, + AutoCloseableReentrantReadWriteLock.WriteLock writeLock) { + lock = mock(AutoCloseableReentrantReadWriteLock.class); + when(lock.readLockAutoCloseable()).thenCallRealMethod(); + when(lock.readLock()).thenReturn(readlock); + when(lock.writeLockAutoCloseable()).thenCallRealMethod(); + when(lock.writeLock()).thenReturn(writeLock); + return lock; + } +} \ No newline at end of file diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java index 14446c1a..eab962ac 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java @@ -20,7 +20,7 @@ void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() { TEST_LOGGER.clear(); OpenFeatureAPI api = mock(OpenFeatureAPI.class); when(api.getProvider()).thenReturn(new DoSomethingProvider()); - when(api.getApiHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook())); + when(api.getHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook())); OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");