diff --git a/CHANGELOG.md b/CHANGELOG.md index b9ba52f58a..1c081e0398 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/FeatureFlags.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/FeatureFlags.kt index efb57506ec..24de0276b2 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/FeatureFlags.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/FeatureFlags.kt @@ -1,42 +1,113 @@ package com.bugsnag.android import java.io.IOException +import kotlin.math.max -internal class FeatureFlags( - internal val store: MutableMap = mutableMapOf() +internal class FeatureFlags private constructor( + @Volatile + private var flags: Array ) : 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()) + + 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) { - featureFlags.forEach { (name, variant) -> - addFeatureFlag(name, variant) + override fun addFeatureFlags(featureFlags: Iterable) { + synchronized(this) { + val flagArray = flags + + val newFlags = ArrayList( + // 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(flagArray.size - 1) + flagArray.copyInto(out, 0, 0, index) + flagArray.copyInto(out, index, index + 1) + + @Suppress("UNCHECKED_CAST") + flags = out as Array + } } - @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() @@ -44,9 +115,7 @@ internal class FeatureFlags( stream.endArray() } - @Synchronized fun toList(): List = store.entries.map { (name, variant) -> - FeatureFlag(name, variant.takeUnless { it == emptyVariant }) - } + fun toList(): List = flags.map { (name, variant) -> FeatureFlag(name, variant) } - @Synchronized fun copy() = FeatureFlags(store.toMutableMap()) + fun copy() = FeatureFlags(flags) } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/FeatureFlagsTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/FeatureFlagsTest.kt new file mode 100644 index 0000000000..f44d4114ae --- /dev/null +++ b/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(), 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() + ) + } +}