feat(feature flags): don't copy FeatureFlags on the crashing thread

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## TBD
+
+### Enhancements
+
+* FeatureFlags are now a copy-on-write structure, and so don't need to be defensive copied on a crashing thread
+  [#2005](https://github.com/bugsnag/bugsnag-android/pull/2005)
+
 ## 6.3.0 (2024-03-19)
 
 ### Enhancements

bugsnag-android-core/src/main/java/com/bugsnag/android/FeatureFlags.kt

@@ -1,52 +1,121 @@
 package com.bugsnag.android
 
 import java.io.IOException
+import kotlin.math.max
 
-internal class FeatureFlags(
-    internal val store: MutableMap<String, String?> = mutableMapOf()
+internal class FeatureFlags private constructor(
+    @Volatile
+    private var flags: Array<FeatureFlag>
 ) : JsonStream.Streamable, FeatureFlagAware {
-    private val emptyVariant = "__EMPTY_VARIANT_SENTINEL__"
 
-    @Synchronized override fun addFeatureFlag(name: String) {
+    /*
+     * Implemented as *effectively* a CopyOnWriteArrayList - but since FeatureFlags are
+     * key/value pairs, CopyOnWriteArrayList would require external locking (in addition to it's
+     * internal locking) for us to be sure we are not adding duplicates.
+     *
+     * This class aims to have similar performance while also ensuring that the FeatureFlag object
+     * themselves don't leak, as they are mutable and we want 'copy' to be an O(1) snapshot
+     * operation for when an Event is created.
+     *
+     * It's assumed that *most* FeatureFlags will be added up-front, or during the normal app
+     * lifecycle (not during an Event).
+     *
+     * As such a copy-on-write structure allows an Event to simply capture a reference to the
+     * "snapshot" of FeatureFlags that were active when the Event was created.
+     */
+
+    constructor() : this(emptyArray<FeatureFlag>())
+
+    override fun addFeatureFlag(name: String) {
         addFeatureFlag(name, null)
     }
 
-    @Synchronized override fun addFeatureFlag(name: String, variant: String?) {
-        store[name] = variant ?: emptyVariant
+    override fun addFeatureFlag(name: String, variant: String?) {
+        synchronized(this) {
+            val flagArray = flags
+            val index = flagArray.indexOfFirst { it.name == name }
+
+            flags = when {
+                // this is a new FeatureFlag
+                index == -1 -> flagArray + FeatureFlag(name, variant)
+
+                // this is a change to an existing FeatureFlag
+                flagArray[index].variant != variant -> flagArray.copyOf().also {
+                    // replace the existing FeatureFlag in-place
+                    it[index] = FeatureFlag(name, variant)
+                }
+
+                // no actual change, so we return
+                else -> return
+            }
+        }
     }
 
-    @Synchronized override fun addFeatureFlags(featureFlags: Iterable<FeatureFlag>) {
-        featureFlags.forEach { (name, variant) ->
-            addFeatureFlag(name, variant)
+    override fun addFeatureFlags(featureFlags: Iterable<FeatureFlag>) {
+        synchronized(this) {
+            val flagArray = flags
+
+            val newFlags = ArrayList<FeatureFlag>(
+                // try to guess a reasonable upper-bound for the output array
+                if (featureFlags is Collection<*>) flagArray.size + featureFlags.size
+                else max(flagArray.size * 2, flagArray.size)
+            )
+
+            newFlags.addAll(flagArray)
+
+            featureFlags.forEach { (name, variant) ->
+                val existingIndex = newFlags.indexOfFirst { it.name == name }
+                when (existingIndex) {
+                    // add a new flag to the end of the list
+                    -1 -> newFlags.add(FeatureFlag(name, variant))
+                    // replace the existing flag
+                    else -> newFlags[existingIndex] = FeatureFlag(name, variant)
+                }
+            }
+
+            flags = newFlags.toTypedArray()
         }
     }
 
-    @Synchronized override fun clearFeatureFlag(name: String) {
-        store.remove(name)
+    override fun clearFeatureFlag(name: String) {
+        synchronized(this) {
+            val flagArray = flags
+            val index = flagArray.indexOfFirst { it.name == name }
+            if (index == -1) {
+                return
+            }
+
+            val out = arrayOfNulls<FeatureFlag>(flagArray.size - 1)
+            flagArray.copyInto(out, 0, 0, index)
+            flagArray.copyInto(out, index, index + 1)
+
+            @Suppress("UNCHECKED_CAST")
+            flags = out as Array<FeatureFlag>
+        }
     }
 
-    @Synchronized override fun clearFeatureFlags() {
-        store.clear()
+    override fun clearFeatureFlags() {
+        synchronized(this) {
+            flags = emptyArray()
+        }
     }
 
     @Throws(IOException::class)
     override fun toStream(stream: JsonStream) {
-        val storeCopy = synchronized(this) { store.toMap() }
+        val storeCopy = flags
         stream.beginArray()
         storeCopy.forEach { (name, variant) ->
             stream.beginObject()
             stream.name("featureFlag").value(name)
-            if (variant != emptyVariant) {
+            if (variant != null) {
                 stream.name("variant").value(variant)
             }
             stream.endObject()
         }
         stream.endArray()
     }
 
-    @Synchronized fun toList(): List<FeatureFlag> = store.entries.map { (name, variant) ->
-        FeatureFlag(name, variant.takeUnless { it == emptyVariant })
-    }
+    fun toList(): List<FeatureFlag> = flags.map { (name, variant) -> FeatureFlag(name, variant) }
 
-    @Synchronized fun copy() = FeatureFlags(store.toMutableMap())
+    fun copy() = FeatureFlags(flags)
 }

bugsnag-android-core/src/test/java/com/bugsnag/android/FeatureFlagsTest.kt

@@ -0,0 +1,127 @@
+package com.bugsnag.android
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotSame
+import org.junit.Before
+import org.junit.Test
+
+class FeatureFlagsTest {
+    private lateinit var flags: FeatureFlags
+
+    @Before
+    fun createEmptyFeatureFlags() {
+        flags = FeatureFlags()
+    }
+
+    @Test
+    fun addDistinctFeatureFlag() {
+        flags.addFeatureFlag("empty")
+        flags.addFeatureFlag("keyWith", "value")
+        flags.addFeatureFlag("otherKey", "another value")
+
+        assertEquals(
+            listOf(
+                FeatureFlag("empty"),
+                FeatureFlag("keyWith", "value"),
+                FeatureFlag("otherKey", "another value")
+            ),
+            flags.toList()
+        )
+    }
+
+    @Test
+    fun overwriteFeatureFlags() {
+        flags.addFeatureFlag("empty")
+        flags.addFeatureFlag("keyWith", "value")
+        flags.addFeatureFlag("otherKey", "another value")
+
+        flags.addFeatureFlag("empty")
+        flags.addFeatureFlag("keyWith", "overwrite value")
+        flags.addFeatureFlag("newKey", "new value")
+
+        flags.clearFeatureFlag("otherKey")
+        flags.clearFeatureFlag("no such key")
+
+        assertEquals(
+            listOf(
+                FeatureFlag("empty"),
+                FeatureFlag("keyWith", "overwrite value"),
+                FeatureFlag("newKey", "new value")
+            ),
+            flags.toList()
+        )
+    }
+
+    @Test
+    fun clearFeatureFlags() {
+        // make sure clearing an empty table doesn't break anything
+        flags.clearFeatureFlags()
+
+        flags.addFeatureFlag("empty")
+        flags.addFeatureFlag("keyWith", "value")
+        flags.addFeatureFlag("otherKey", "another value")
+
+        flags.clearFeatureFlags()
+
+        assertEquals(emptyList<FeatureFlag>(), flags.toList())
+
+        // make sure that clearing the list doesn't break adding new flags afterwards
+        flags.addFeatureFlag("empty key")
+        flags.addFeatureFlag("keyWith", "magic value")
+        flags.addFeatureFlag("newKey", "new value")
+
+        assertEquals(
+            listOf(
+                FeatureFlag("empty key"),
+                FeatureFlag("keyWith", "magic value"),
+                FeatureFlag("newKey", "new value")
+            ),
+            flags.toList()
+        )
+    }
+
+    @Test
+    fun addFeatureFlags() {
+        flags.addFeatureFlag("empty")
+        flags.addFeatureFlag("key1", "value1")
+        flags.addFeatureFlag("key2", "value2")
+
+        flags.addFeatureFlags(
+            listOf(
+                FeatureFlag("key1", "overwrite value"),
+                FeatureFlag("bulk key1", "bulk value1"),
+                FeatureFlag("bulk key2", "bulk value2"),
+            )
+        )
+
+        flags.addFeatureFlags(
+            sequence {
+                repeat(5) { index ->
+                    yield(FeatureFlag("feature $index"))
+                }
+            }.asIterable()
+        )
+
+        val cloned = flags.copy()
+        assertNotSame(flags, cloned)
+
+        // add an extra value and make sure it doesn't show up in 'cloned'
+        flags.addFeatureFlag("extra value", "extra value")
+
+        assertEquals(
+            listOf(
+                FeatureFlag("empty"),
+                FeatureFlag("key1", "overwrite value"),
+                FeatureFlag("key2", "value2"),
+                FeatureFlag("bulk key1", "bulk value1"),
+                FeatureFlag("bulk key2", "bulk value2"),
+                FeatureFlag("feature 0"),
+                FeatureFlag("feature 1"),
+                FeatureFlag("feature 2"),
+                FeatureFlag("feature 3"),
+                FeatureFlag("feature 4")
+            ),
+            cloned.toList()
+        )
+    }
+}