fix(FeatureFlag): added a ReadWriteLock to the FeatureFlagStore, and replaced FeatureFlag constructor with of factory method

lemnik · lemnik · commit d06aa20ce03d · 2026-03-12T14:28:51.000Z
diff --git a/bugsnag/src/main/java/com/bugsnag/FeatureFlag.java b/bugsnag/src/main/java/com/bugsnag/FeatureFlag.java
@@ -13,22 +13,13 @@ public class FeatureFlag {
     private final String name;
     private final String variant;
 
-    /**
-     * Create a feature flag with a name and no variant.
-     *
-     * @param name the name of the feature flag
-     */
-    public FeatureFlag(String name) {
-        this(name, null);
-    }
-
     /**
      * Create a feature flag with a name and variant.
      *
-     * @param name the name of the feature flag
+     * @param name    the name of the feature flag
      * @param variant the variant of the feature flag (can be null)
      */
-    public FeatureFlag(String name, String variant) {
+    private FeatureFlag(String name, String variant) {
         if (name == null || name.isEmpty()) {
             throw new IllegalArgumentException("Feature flag name cannot be null or empty");
         }
@@ -77,4 +68,31 @@ public int hashCode() {
     public String toString() {
         return "FeatureFlag{name='" + name + "', variant='" + variant + "'}";
     }
+
+    /**
+     * Create a feature flag with a name and no vairant.
+     *
+     * @param name the name of the feature flag
+     */
+    public static FeatureFlag of(String name) {
+        if (name == null || name.isEmpty()) {
+            return null;
+        }
+
+        return new FeatureFlag(name, null);
+    }
+
+    /**
+     * Create a feature flag with a name and variant.
+     *
+     * @param name    the name of the feature flag
+     * @param variant the variant of the feature flag (can be null)
+     */
+    public static FeatureFlag of(String name, String variant) {
+        if (name == null || name.isEmpty()) {
+            return null;
+        }
+
+        return new FeatureFlag(name, variant);
+    }
 }
diff --git a/bugsnag/src/main/java/com/bugsnag/FeatureFlagStore.java b/bugsnag/src/main/java/com/bugsnag/FeatureFlagStore.java
@@ -5,6 +5,8 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * Internal storage for feature flags that maintains insertion order.
@@ -13,19 +15,25 @@
 class FeatureFlagStore {
     // LinkedHashMap maintains insertion order
     private final Map<String, String> flags = new LinkedHashMap<String, String>();
+    private final ReadWriteLock lock = new ReentrantReadWriteLock();
 
     /**
      * Add a feature flag with the specified name and variant.
      * If the name already exists, the variant will be updated without changing position.
      *
-     * @param name the feature flag name
+     * @param name    the feature flag name
      * @param variant the feature flag variant (can be null)
      */
-    synchronized void addFeatureFlag(String name, String variant) {
+    void addFeatureFlag(String name, String variant) {
         if (name == null || name.isEmpty()) {
             return;
         }
-        flags.put(name, variant);
+        lock.writeLock().lock();
+        try {
+            flags.put(name, variant);
+        } finally {
+            lock.writeLock().unlock();
+        }
     }
 
     /**
@@ -34,14 +42,20 @@ synchronized void addFeatureFlag(String name, String variant) {
      *
      * @param featureFlags the feature flags to add
      */
-    synchronized void addFeatureFlags(Collection<FeatureFlag> featureFlags) {
-        if (featureFlags == null) {
+    void addFeatureFlags(Collection<FeatureFlag> featureFlags) {
+        if (featureFlags == null || featureFlags.isEmpty()) {
             return;
         }
-        for (FeatureFlag flag : featureFlags) {
-            if (flag != null) {
-                addFeatureFlag(flag.getName(), flag.getVariant());
+
+        lock.writeLock().lock();
+        try {
+            for (FeatureFlag flag : featureFlags) {
+                if (flag != null) {
+                    flags.put(flag.getName(), flag.getVariant());
+                }
             }
+        } finally {
+            lock.writeLock().unlock();
         }
     }
 
@@ -50,40 +64,63 @@ synchronized void addFeatureFlags(Collection<FeatureFlag> featureFlags) {
      *
      * @param name the feature flag name to remove
      */
-    synchronized void clearFeatureFlag(String name) {
+    void clearFeatureFlag(String name) {
         if (name != null) {
-            flags.remove(name);
+            lock.writeLock().lock();
+            try {
+                flags.remove(name);
+            } finally {
+                lock.writeLock().unlock();
+            }
         }
     }
 
     /**
      * Remove all feature flags.
      */
-    synchronized void clearFeatureFlags() {
-        flags.clear();
+    void clearFeatureFlags() {
+        lock.writeLock().lock();
+        try {
+            flags.clear();
+        } finally {
+            lock.writeLock().unlock();
+        }
     }
 
     /**
      * Get a list of all feature flags in insertion order.
      *
      * @return an unmodifiable list of feature flags
      */
-    synchronized List<FeatureFlag> toList() {
-        List<FeatureFlag> result = new ArrayList<FeatureFlag>(flags.size());
-        for (Map.Entry<String, String> entry : flags.entrySet()) {
-            result.add(new FeatureFlag(entry.getKey(), entry.getValue()));
+    List<FeatureFlag> toList() {
+        lock.readLock().lock();
+        try {
+            List<FeatureFlag> result = new ArrayList<>(flags.size());
+            for (Map.Entry<String, String> entry : flags.entrySet()) {
+                FeatureFlag flag = FeatureFlag.of(entry.getKey(), entry.getValue());
+                if (flag != null) {
+                    result.add(flag);
+                }
+            }
+            return result;
+        } finally {
+            lock.readLock().unlock();
         }
-        return result;
     }
 
     /**
      * Create a copy of this store with all the same flags.
      *
      * @return a new FeatureFlagStore with the same flags
      */
-    synchronized FeatureFlagStore copy() {
+    FeatureFlagStore copy() {
         FeatureFlagStore copy = new FeatureFlagStore();
-        copy.flags.putAll(this.flags);
+        lock.readLock().lock();
+        try {
+            copy.flags.putAll(this.flags);
+        } finally {
+            lock.readLock().unlock();
+        }
         return copy;
     }
 
@@ -94,14 +131,29 @@ synchronized FeatureFlagStore copy() {
      *
      * @param other the other store to merge from
      */
-    synchronized void merge(FeatureFlagStore other) {
-        if (other == null) {
+    void merge(FeatureFlagStore other) {
+        if (other == null || other == this) {
             return;
         }
-        synchronized (other) {
-            for (Map.Entry<String, String> entry : other.flags.entrySet()) {
-                flags.put(entry.getKey(), entry.getValue());
+
+        // Warning: this *looks* like a classic deadlock pattern, but because this method is only ever called
+        // with isolated copies of FeatureFlagStore, the locks will never actually be contended.
+        // If this method were to be called with two live stores, then it would be possible for a deadlock to occur.
+        other.lock.readLock().lock();
+        try {
+            // we don't use other.size() because it grabs a lock.readLock() again
+            if (other.flags.isEmpty()) {
+                return;
             }
+
+            lock.writeLock().lock();
+            try {
+                flags.putAll(other.flags);
+            } finally {
+                lock.writeLock().unlock();
+            }
+        } finally {
+            other.lock.readLock().unlock();
         }
     }
 
@@ -110,7 +162,12 @@ synchronized void merge(FeatureFlagStore other) {
      *
      * @return the number of feature flags
      */
-    synchronized int size() {
-        return flags.size();
+    int size() {
+        lock.readLock().lock();
+        try {
+            return flags.size();
+        } finally {
+            lock.readLock().unlock();
+        }
     }
 }
diff --git a/bugsnag/src/test/java/com/bugsnag/BugsnagFeatureFlagTest.java b/bugsnag/src/test/java/com/bugsnag/BugsnagFeatureFlagTest.java
@@ -53,8 +53,8 @@ public void testAddFeatureFlagWithoutVariant() {
     @Test
     public void testAddFeatureFlags() {
         List<FeatureFlag> flagsToAdd = new ArrayList<FeatureFlag>();
-        flagsToAdd.add(new FeatureFlag("flag1", "variant-a"));
-        flagsToAdd.add(new FeatureFlag("flag2", "variant-b"));
+        flagsToAdd.add(FeatureFlag.of("flag1", "variant-a"));
+        flagsToAdd.add(FeatureFlag.of("flag2", "variant-b"));
 
         bugsnag.addFeatureFlags(flagsToAdd);
 
diff --git a/bugsnag/src/test/java/com/bugsnag/ConfigurationFeatureFlagTest.java b/bugsnag/src/test/java/com/bugsnag/ConfigurationFeatureFlagTest.java
@@ -48,8 +48,8 @@ public void testAddFeatureFlagWithoutVariant() {
     @Test
     public void testAddFeatureFlags() {
         List<FeatureFlag> flagsToAdd = new ArrayList<FeatureFlag>();
-        flagsToAdd.add(new FeatureFlag("flag1", "variant-a"));
-        flagsToAdd.add(new FeatureFlag("flag2", "variant-b"));
+        flagsToAdd.add(FeatureFlag.of("flag1", "variant-a"));
+        flagsToAdd.add(FeatureFlag.of("flag2", "variant-b"));
 
         config.addFeatureFlags(flagsToAdd);
 
diff --git a/bugsnag/src/test/java/com/bugsnag/FeatureFlagStoreTest.java b/bugsnag/src/test/java/com/bugsnag/FeatureFlagStoreTest.java
@@ -78,8 +78,8 @@ public void testAddFeatureFlagUpdateDoesNotChangeOrder() {
     @Test
     public void testAddFeatureFlags() {
         List<FeatureFlag> flagsToAdd = new ArrayList<FeatureFlag>();
-        flagsToAdd.add(new FeatureFlag("flag1", "variant-a"));
-        flagsToAdd.add(new FeatureFlag("flag2", "variant-b"));
+        flagsToAdd.add(FeatureFlag.of("flag1", "variant-a"));
+        flagsToAdd.add(FeatureFlag.of("flag2", "variant-b"));
 
         store.addFeatureFlags(flagsToAdd);
 
diff --git a/bugsnag/src/test/java/com/bugsnag/FeatureFlagTest.java b/bugsnag/src/test/java/com/bugsnag/FeatureFlagTest.java
@@ -13,41 +13,41 @@ public class FeatureFlagTest {
 
     @Test
     public void testFeatureFlagWithVariant() {
-        FeatureFlag flag = new FeatureFlag("test-flag", "variant-a");
+        FeatureFlag flag = FeatureFlag.of("test-flag", "variant-a");
         assertEquals("test-flag", flag.getName());
         assertEquals("variant-a", flag.getVariant());
     }
 
     @Test
     public void testFeatureFlagWithoutVariant() {
-        FeatureFlag flag = new FeatureFlag("test-flag");
+        FeatureFlag flag = FeatureFlag.of("test-flag");
         assertEquals("test-flag", flag.getName());
         assertNull(flag.getVariant());
     }
 
     @Test
     public void testFeatureFlagWithNullVariant() {
-        FeatureFlag flag = new FeatureFlag("test-flag", null);
+        FeatureFlag flag = FeatureFlag.of("test-flag", null);
         assertEquals("test-flag", flag.getName());
         assertNull(flag.getVariant());
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void testFeatureFlagWithNullName() {
-        new FeatureFlag(null);
+        assertNull(FeatureFlag.of(null));
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void testFeatureFlagWithEmptyName() {
-        new FeatureFlag("");
+        assertNull(FeatureFlag.of(""));
     }
 
     @Test
     public void testFeatureFlagEquals() {
-        FeatureFlag flag1 = new FeatureFlag("test", "variant-a");
-        FeatureFlag flag2 = new FeatureFlag("test", "variant-a");
-        FeatureFlag flag3 = new FeatureFlag("test", "variant-b");
-        FeatureFlag flag4 = new FeatureFlag("other", "variant-a");
+        FeatureFlag flag1 = FeatureFlag.of("test", "variant-a");
+        FeatureFlag flag2 = FeatureFlag.of("test", "variant-a");
+        FeatureFlag flag3 = FeatureFlag.of("test", "variant-b");
+        FeatureFlag flag4 = FeatureFlag.of("other", "variant-a");
 
         assertEquals(flag1, flag2);
         assertTrue(!flag1.equals(flag3));
@@ -56,14 +56,14 @@ public void testFeatureFlagEquals() {
 
     @Test
     public void testFeatureFlagHashCode() {
-        FeatureFlag flag1 = new FeatureFlag("test", "variant-a");
-        FeatureFlag flag2 = new FeatureFlag("test", "variant-a");
+        FeatureFlag flag1 = FeatureFlag.of("test", "variant-a");
+        FeatureFlag flag2 = FeatureFlag.of("test", "variant-a");
         assertEquals(flag1.hashCode(), flag2.hashCode());
     }
 
     @Test
     public void testFeatureFlagToString() {
-        FeatureFlag flag = new FeatureFlag("test-flag", "variant-a");
+        FeatureFlag flag = FeatureFlag.of("test-flag", "variant-a");
         String result = flag.toString();
         assertTrue(result.contains("test-flag"));
         assertTrue(result.contains("variant-a"));
diff --git a/bugsnag/src/test/java/com/bugsnag/ReportFeatureFlagTest.java b/bugsnag/src/test/java/com/bugsnag/ReportFeatureFlagTest.java
@@ -53,8 +53,8 @@ public void testAddFeatureFlagWithoutVariant() {
     @Test
     public void testAddFeatureFlags() {
         List<FeatureFlag> flagsToAdd = new ArrayList<FeatureFlag>();
-        flagsToAdd.add(new FeatureFlag("flag1", "variant-a"));
-        flagsToAdd.add(new FeatureFlag("flag2", "variant-b"));
+        flagsToAdd.add(FeatureFlag.of("flag1", "variant-a"));
+        flagsToAdd.add(FeatureFlag.of("flag2", "variant-b"));
 
         Report report = bugsnag.buildReport(new RuntimeException("Test"));
         report.addFeatureFlags(flagsToAdd);
