feat!: add rw locks to client/api, hook accessor name (#131)

* fix: add read/write locks to client/api

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* dont lock entire evaluation

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* add tests

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* fixup comment

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* fixup pom comment

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* increase lock granularity, imporove tests

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* fix spotbugs

Signed-off-by: Todd Baert <toddbaert@gmail.com>

* remove commented test

Signed-off-by: Todd Baert <toddbaert@gmail.com>

Signed-off-by: Todd Baert <toddbaert@gmail.com>

pom.xml

@@ -164,6 +164,21 @@
   <build>
     <plugins>
 
+      <plugin>
+        <groupId>org.codehaus.mojo</groupId>
+        <artifactId>build-helper-maven-plugin</artifactId>
+        <version>3.3.0</version>
+        <executions>
+          <execution>
+            <phase>validate</phase>
+            <id>get-cpu-count</id>
+            <goals>
+              <goal>cpu-count</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+
       <plugin>
         <groupId>org.cyclonedx</groupId>
         <artifactId>cyclonedx-maven-plugin</artifactId>
@@ -231,6 +246,9 @@
           <argLine>
             ${surefireArgLine}
           </argLine>
+          <!-- fork a new JVM to isolate test suites, especially important with singletons -->
+          <forkCount>${cpu.count}</forkCount>
+          <reuseForks>false</reuseForks>
           <excludes>
             <!-- tests to exclude -->
             <exclude>${testExclusions}</exclude>

spotbugs-exclusions.xml

@@ -9,11 +9,26 @@
         <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
         <Bug pattern="MS_EXPOSE_REP"/>
     </And>
-    <!-- similarly, client using the singleton doesn't seem bad -->
+    <!-- evaluation context and hooks are mutable if mutable impl is used -->
+    <And>
+        <Class name="dev.openfeature.sdk.OpenFeatureClient"/>
+        <Bug pattern="EI_EXPOSE_REP"/>
+    </And>
     <And>
         <Class name="dev.openfeature.sdk.OpenFeatureClient"/>
         <Bug pattern="EI_EXPOSE_REP2"/>
     </And>
+    <And>
+        <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
+        <Bug pattern="EI_EXPOSE_REP"/>
+    </And>
+    <And>
+        <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </And>
+
+
+
 
     <!-- Test class that should be excluded -->
     <Match>

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<Hook> getClientHooks();
+    List<Hook> getHooks();
 }

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;
+    // package-private multi-read/single-write lock
-    @Getter
+    static AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock();
-    @Setter
+    static AutoCloseableReentrantReadWriteLock providerLock = new AutoCloseableReentrantReadWriteLock();
+    static AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock();
     private FeatureProvider provider;
-    @Getter
-    @Setter
     private EvaluationContext evaluationContext;
-    @Getter
     private List<Hook> 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) {
+        return SingletonHolder.INSTANCE;
-            if (api == null) {
-                api = new OpenFeatureAPI();
-            }
-        }
-        return api;
     }
 
     public Metadata getProviderMetadata() {
@@ -56,11 +54,66 @@ public class OpenFeatureAPI {
         return new OpenFeatureClient(this, name, version);
     }
 
-    public void addHooks(Hook... hooks) {
+    /**
-        this.apiHooks.addAll(Arrays.asList(hooks));
+     * {@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) {
+        try (AutoCloseableLock __ = hooksLock.writeLockAutoCloseable()) {
+            this.apiHooks.addAll(Arrays.asList(hooks));
+        }
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public List<Hook> getHooks() {
+        try (AutoCloseableLock __ = hooksLock.readLockAutoCloseable()) {
+            return this.apiHooks;
+        }
+    }
+
+    /**
+     * {@inheritDoc}
+     */
     public void clearHooks() {
+        try (AutoCloseableLock __ = hooksLock.writeLockAutoCloseable()) {
             this.apiHooks.clear();
         }
     }
+}

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

@@ -8,9 +8,10 @@ import java.util.Map;
 
 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<Hook> clientHooks;
     private final HookSupport hookSupport;
-
+    AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock();
-    @Getter
+    AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock();
-    @Setter
     private EvaluationContext evaluationContext;
 
     /**
@@ -46,10 +45,45 @@ public class OpenFeatureClient implements Client {
         this.hookSupport = new HookSupport();
     }
 
+    /**
+     * {@inheritDoc}
+     */
     @Override
     public void addHooks(Hook... hooks) {
+        try (AutoCloseableLock __ = this.hooksLock.writeLockAutoCloseable()) {
             this.clientHooks.addAll(Arrays.asList(hooks));
         }
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<Hook> 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 <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key, T defaultValue,
             EvaluationContext ctx, FlagEvaluationOptions options) {
@@ -57,34 +91,41 @@ public class OpenFeatureClient implements Client {
                 () -> FlagEvaluationOptions.builder().build());
         Map<String, Object> 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<T> details = null;
         List<Hook> mergedHooks = null;
         HookContext<T> hookCtx = null;
+        FeatureProvider provider = null;
 
         try {
+            final EvaluationContext apiContext;
+            final EvaluationContext clientContext;
 
-            hookCtx = HookContext.from(key, type, this.getMetadata(),
+            // openfeatureApi.getProvider() must be called once to maintain a consistent reference
-                    openfeatureApi.getProvider().getMetadata(), ctx, defaultValue);
+            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());
+
+            hookCtx = HookContext.from(key, type, this.getMetadata(),
+                    provider.getMetadata(), ctx, defaultValue);
+
+            // merge of: API.context, client.context, invocation.context
+            apiContext = openfeatureApi.getEvaluationContext() != null
+                    ? openfeatureApi.getEvaluationContext()
+                    : new MutableContext();
+            clientContext = openfeatureApi.getEvaluationContext() != null
+                    ? this.getEvaluationContext()
+                    : new MutableContext();
 
             EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints);
 
             EvaluationContext invocationCtx = ctx.merge(ctxFromHook);
 
-            // merge of: API.context, client.context, invocation.context
-            EvaluationContext apiContext = openfeatureApi.getEvaluationContext() != null
-                    ? openfeatureApi.getEvaluationContext()
-                    : new MutableContext();
-            EvaluationContext clientContext = openfeatureApi.getEvaluationContext() != null
-                    ? this.getEvaluationContext()
-                    : new MutableContext();
             EvaluationContext mergedCtx = apiContext.merge(clientContext.merge(invocationCtx));
 
             ProviderEvaluation<T> providerEval = (ProviderEvaluation<T>) createProviderEvaluation(type, key,

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();
+}

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;
+    }
+}

src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java

@@ -6,12 +6,14 @@ import static org.mockito.ArgumentMatchers.any;
 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);
+    }
 }

src/test/java/dev/openfeature/sdk/DoSomethingProvider.java

@@ -2,6 +2,7 @@ package dev.openfeature.sdk;
 
 public class DoSomethingProvider implements FeatureProvider {
 
+    public static final String name = "Something";
     private EvaluationContext savedContext;
 
     public EvaluationContext getMergedContext() {
@@ -10,7 +11,7 @@ public class DoSomethingProvider implements FeatureProvider {
 
     @Override
     public Metadata getMetadata() {
-        return () -> "test";
+        return () -> name;
     }
 
     @Override

src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java

@@ -53,7 +53,7 @@ class FlagEvaluationSpecTest implements HookFixtures {
     @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 @@ class FlagEvaluationSpecTest implements HookFixtures {
         OpenFeatureAPI api = OpenFeatureAPI.getInstance();
         api.addHooks(h1);
 
-        assertEquals(1, api.getApiHooks().size());
+        assertEquals(1, api.getHooks().size());
-        assertEquals(h1, api.getApiHooks().get(0));
+        assertEquals(h1, api.getHooks().get(0));
 
         api.addHooks(h2);
-        assertEquals(2, api.getApiHooks().size());
+        assertEquals(2, api.getHooks().size());
-        assertEquals(h2, api.getApiHooks().get(1));
+        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 @@ class FlagEvaluationSpecTest implements HookFixtures {
         Hook m2 = mock(Hook.class);
         c.addHooks(m1);
         c.addHooks(m2);
-        List<Hook> hooks = c.getClientHooks();
+        List<Hook> hooks = c.getHooks();
         assertEquals(2, hooks.size());
         assertTrue(hooks.contains(m1));
         assertTrue(hooks.contains(m2));

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;
+    }
+}

src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java

@@ -20,7 +20,7 @@ class OpenFeatureClientTest implements HookFixtures {
         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");