Supports merging across scope types (GLOBAL, ISOLATION, CURRENT)
*
+ *
+ * {@link #flags} always holds an immutable list. Writers hold {@link #lock} and swap in a new
+ * list; readers take a single volatile read and are then free to iterate or index into a list that
+ * can no longer change. This is what makes {@link #clone()} free: the copy shares the list rather
+ * than duplicating it.
*/
@ApiStatus.Internal
public final class FeatureFlagBuffer implements IFeatureFlagBuffer {
- private volatile @NotNull CopyOnWriteArrayList flags;
+ private volatile @NotNull List flags;
private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
- private int maxSize;
+ private final int maxSize;
- private FeatureFlagBuffer(int maxSize) {
- this.maxSize = maxSize;
- this.flags = new CopyOnWriteArrayList<>();
+ private FeatureFlagBuffer(final int maxSize) {
+ this(maxSize, Collections.emptyList());
}
- private FeatureFlagBuffer(
- int maxSize, final @NotNull CopyOnWriteArrayList flags) {
+ private FeatureFlagBuffer(final int maxSize, final @NotNull List flags) {
this.maxSize = maxSize;
this.flags = flags;
}
- private FeatureFlagBuffer(@NotNull FeatureFlagBuffer other) {
- this.maxSize = other.maxSize;
- this.flags = new CopyOnWriteArrayList<>(other.flags);
- }
-
@Override
public void add(final @Nullable String flag, final @Nullable Boolean result) {
if (flag == null || result == null) {
return;
}
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
- final int size = flags.size();
- for (int i = 0; i < size; i++) {
- final @NotNull FeatureFlagEntry entry = flags.get(i);
- if (entry.flag.equals(flag)) {
- flags.remove(i);
- break;
+ final @NotNull List current = flags;
+ final @NotNull List updated = new ArrayList<>(current.size() + 1);
+ for (final @NotNull FeatureFlagEntry entry : current) {
+ if (!entry.flag.equals(flag)) {
+ updated.add(entry);
}
}
- flags.add(new FeatureFlagEntry(flag, result, System.nanoTime()));
+ updated.add(new FeatureFlagEntry(flag, result, System.nanoTime()));
- if (flags.size() > maxSize) {
- flags.remove(0);
+ if (updated.size() > maxSize) {
+ updated.remove(0);
}
+
+ flags = Collections.unmodifiableList(updated);
}
}
@Override
public void clear() {
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
- flags.clear();
+ flags = Collections.emptyList();
}
}
@Override
public @Nullable FeatureFlags getFeatureFlags() {
- List featureFlags = new ArrayList<>();
- for (FeatureFlagEntry entry : flags) {
+ final @NotNull List snapshot = flags;
+ final @NotNull List featureFlags = new ArrayList<>(snapshot.size());
+ for (final @NotNull FeatureFlagEntry entry : snapshot) {
featureFlags.add(entry.toFeatureFlag());
}
return new FeatureFlags(featureFlags);
@@ -87,7 +88,7 @@ public void clear() {
@Override
public @NotNull IFeatureFlagBuffer clone() {
- return new FeatureFlagBuffer(this);
+ return new FeatureFlagBuffer(maxSize, flags);
}
public static @NotNull IFeatureFlagBuffer create(final @NotNull SentryOptions options) {
@@ -123,6 +124,9 @@ public void clear() {
* If a duplicate is found we skip it since we're iterating in reverse order and we already
* have the latest entry.
*
+ *
Entries carrying the same timestamp are resolved in favour of the most specific scope, so
+ * CURRENT wins over ISOLATION, which in turn wins over GLOBAL.
+ *
* @param maxSize max number of feature flags
* @param globalBuffer buffer from global scope
* @param isolationBuffer buffer from isolation scope
@@ -135,12 +139,13 @@ public void clear() {
final @Nullable FeatureFlagBuffer isolationBuffer,
final @Nullable FeatureFlagBuffer currentBuffer) {
- // Capture references to avoid inconsistencies from concurrent modifications
- final @Nullable CopyOnWriteArrayList globalFlags =
+ // One volatile read each pins an immutable list, so concurrent writers cannot shift these
+ // out from under the index arithmetic below
+ final @Nullable List globalFlags =
globalBuffer == null ? null : globalBuffer.flags;
- final @Nullable CopyOnWriteArrayList isolationFlags =
+ final @Nullable List isolationFlags =
isolationBuffer == null ? null : isolationBuffer.flags;
- final @Nullable CopyOnWriteArrayList currentFlags =
+ final @Nullable List currentFlags =
currentBuffer == null ? null : currentBuffer.flags;
final int globalSize = globalFlags == null ? 0 : globalFlags.size();
@@ -166,8 +171,7 @@ public void clear() {
FeatureFlagEntry currentEntry =
currentFlags == null || currentIndex < 0 ? null : currentFlags.get(currentIndex);
- final @NotNull java.util.Map uniqueFlags =
- new java.util.LinkedHashMap<>(maxSize);
+ final @NotNull Map uniqueFlags = new LinkedHashMap<>(maxSize);
// check if there is still room and remaining items to check
while (uniqueFlags.size() < maxSize
@@ -177,66 +181,59 @@ public void clear() {
@Nullable ScopeType selectedBuffer = null;
// choose newest entry across all buffers
- if (globalEntry != null && (entryToAdd == null || globalEntry.nanos > entryToAdd.nanos)) {
+ if (globalEntry != null) {
entryToAdd = globalEntry;
selectedBuffer = ScopeType.GLOBAL;
}
if (isolationEntry != null
- && (entryToAdd == null || isolationEntry.nanos > entryToAdd.nanos)) {
+ && (entryToAdd == null || isolationEntry.nanos >= entryToAdd.nanos)) {
entryToAdd = isolationEntry;
selectedBuffer = ScopeType.ISOLATION;
}
- if (currentEntry != null && (entryToAdd == null || currentEntry.nanos > entryToAdd.nanos)) {
+ if (currentEntry != null && (entryToAdd == null || currentEntry.nanos >= entryToAdd.nanos)) {
entryToAdd = currentEntry;
selectedBuffer = ScopeType.CURRENT;
}
- if (entryToAdd != null) {
- // no need to update existing entries since we already have the latest
- if (!uniqueFlags.containsKey(entryToAdd.flag)) {
- uniqueFlags.put(entryToAdd.flag, entryToAdd);
- }
-
- // decrement only index of buffer that was selected
- if (ScopeType.CURRENT.equals(selectedBuffer)) {
- currentIndex--;
- currentEntry =
- currentFlags != null && currentIndex >= 0 ? currentFlags.get(currentIndex) : null;
- } else if (ScopeType.ISOLATION.equals(selectedBuffer)) {
- isolationIndex--;
- isolationEntry =
- isolationFlags != null && isolationIndex >= 0
- ? isolationFlags.get(isolationIndex)
- : null;
- } else if (ScopeType.GLOBAL.equals(selectedBuffer)) {
- globalIndex--;
- globalEntry =
- globalFlags != null && globalIndex >= 0 ? globalFlags.get(globalIndex) : null;
- }
- } else {
- // no need to look any further since lists are sorted and we could not find any newer
- // entries anymore
+ if (entryToAdd == null) {
break;
}
+
+ // no need to update existing entries since we already have the latest
+ if (!uniqueFlags.containsKey(entryToAdd.flag)) {
+ uniqueFlags.put(entryToAdd.flag, entryToAdd);
+ }
+
+ // decrement only index of buffer that was selected
+ if (selectedBuffer == ScopeType.CURRENT) {
+ currentIndex--;
+ currentEntry =
+ currentFlags != null && currentIndex >= 0 ? currentFlags.get(currentIndex) : null;
+ } else if (selectedBuffer == ScopeType.ISOLATION) {
+ isolationIndex--;
+ isolationEntry =
+ isolationFlags != null && isolationIndex >= 0
+ ? isolationFlags.get(isolationIndex)
+ : null;
+ } else if (selectedBuffer == ScopeType.GLOBAL) {
+ globalIndex--;
+ globalEntry = globalFlags != null && globalIndex >= 0 ? globalFlags.get(globalIndex) : null;
+ }
}
// Convert to list in reverse order (oldest first, newest last)
final @NotNull List resultList = new ArrayList<>(uniqueFlags.values());
Collections.reverse(resultList);
- return new FeatureFlagBuffer(maxSize, new CopyOnWriteArrayList<>(resultList));
+ return new FeatureFlagBuffer(maxSize, Collections.unmodifiableList(resultList));
}
private static class FeatureFlagEntry {
private final @NotNull String flag;
private final boolean result;
+ private final long nanos;
- @SuppressWarnings("UnusedVariable")
- @NotNull
- private final Long nanos;
-
- public FeatureFlagEntry(
- final @NotNull String flag, final boolean result, final @NotNull Long nanos) {
+ public FeatureFlagEntry(final @NotNull String flag, final boolean result, final long nanos) {
this.flag = flag;
this.result = result;
this.nanos = nanos;
diff --git a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt
index 8ec18ce02b8..ad690634bd6 100644
--- a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt
+++ b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt
@@ -1,6 +1,9 @@
package io.sentry.featureflags
+import com.google.common.truth.Truth.assertThat
import io.sentry.SentryOptions
+import java.util.concurrent.atomic.AtomicBoolean
+import java.util.concurrent.atomic.AtomicReference
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
@@ -347,4 +350,51 @@ class FeatureFlagBufferTest {
val featureFlags = buffer.featureFlags
assertNotNull(featureFlags)
}
+
+ @Test
+ fun `clone does not share later writes with the original`() {
+ val buffer = FeatureFlagBuffer.create(SentryOptions().also { it.maxFeatureFlags = 5 })
+ buffer.add("a", true)
+
+ val clone = buffer.clone()
+ clone.add("b", true)
+ buffer.add("c", true)
+
+ assertThat(buffer.featureFlags!!.values.map { it!!.flag }).containsExactly("a", "c").inOrder()
+ assertThat(clone.featureFlags!!.values.map { it!!.flag }).containsExactly("a", "b").inOrder()
+ }
+
+ @Test
+ fun `merging is safe while another thread adds flags`() {
+ val options = SentryOptions().also { it.maxFeatureFlags = 100 }
+ val globalBuffer = FeatureFlagBuffer.create(options)
+ val isolationBuffer = FeatureFlagBuffer.create(options)
+ val currentBuffer = FeatureFlagBuffer.create(options)
+
+ val stop = AtomicBoolean(false)
+ val writerFailure = AtomicReference(null)
+ // more distinct names than maxFeatureFlags, so adds mix evictions in with refreshes
+ val writer = Thread {
+ try {
+ var i = 0
+ while (!stop.get()) {
+ globalBuffer.add("flag${i++ % 150}", true)
+ }
+ } catch (e: Exception) {
+ writerFailure.set(e)
+ }
+ }
+
+ writer.start()
+ try {
+ repeat(10_000) {
+ FeatureFlagBuffer.merged(options, globalBuffer, isolationBuffer, currentBuffer).featureFlags
+ }
+ } finally {
+ stop.set(true)
+ writer.join()
+ }
+
+ assertThat(writerFailure.get()).isNull()
+ }
}