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

* fix: add read/write locks to client/api Signed-off-by: Todd Baert <[email protected]> * dont lock entire evaluation Signed-off-by: Todd Baert <[email protected]> * add tests Signed-off-by: Todd Baert <[email protected]> * fixup comment Signed-off-by: Todd Baert <[email protected]> * fixup pom comment Signed-off-by: Todd Baert <[email protected]> * increase lock granularity, imporove tests Signed-off-by: Todd Baert <[email protected]> * fix spotbugs Signed-off-by: Todd Baert <[email protected]> * remove commented test Signed-off-by: Todd Baert <[email protected]> Signed-off-by: Todd Baert <[email protected]> Signed-off-by: Bhandari, Pramesh(AWF) <[email protected]>
pbhandari9541 · Nov 3, 2022 · ae140c7 · ae140c7
1 parent fe013fa
commit ae140c7
Show file tree

Hide file tree

Showing 12 changed files with 379 additions and 49 deletions.
diff --git a/pom.xml b/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>

diff --git a/spotbugs-exclusions.xml b/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>

diff --git 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<Hook> getClientHooks();
+    List<Hook> getHooks();
 }
diff --git 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<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) {
-            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<Hook> 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
@@ -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<Hook> 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<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,
@@ -57,34 +91,41 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
                 () -> 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().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<T> providerEval = (ProviderEvaluation<T>) 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
@@ -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
@@ -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;
+    }
+}