diff --git a/CHANGELOG.md b/CHANGELOG.md index 02e1a8a906e..f01d585ae0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- Prevent crashes when feature flags are merged during concurrent scope updates ([#5994](https://github.com/getsentry/sentry-java/pull/5994)) - Prevent duplicated breadcrumbs on tombstone-merged native crash events ([#5888](https://github.com/getsentry/sentry-java/pull/5888)) - Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) diff --git a/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java b/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java index fc696b5948f..2eccde29556 100644 --- a/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java +++ b/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java @@ -27,7 +27,7 @@ @ApiStatus.Internal public final class FeatureFlagBuffer implements IFeatureFlagBuffer { - private volatile @NotNull CopyOnWriteArrayList flags; + private final @NotNull CopyOnWriteArrayList flags; private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); private int maxSize; @@ -135,13 +135,15 @@ public void clear() { final @Nullable FeatureFlagBuffer isolationBuffer, final @Nullable FeatureFlagBuffer currentBuffer) { - // Capture references to avoid inconsistencies from concurrent modifications + // Capture structurally stable snapshots before indexed traversal. Passing a + // CopyOnWriteArrayList directly allows its collection constructor to reuse the immutable + // backing array instead of copying the elements on runtimes that support this optimization. final @Nullable CopyOnWriteArrayList globalFlags = - globalBuffer == null ? null : globalBuffer.flags; + globalBuffer == null ? null : new CopyOnWriteArrayList<>(globalBuffer.flags); final @Nullable CopyOnWriteArrayList isolationFlags = - isolationBuffer == null ? null : isolationBuffer.flags; + isolationBuffer == null ? null : new CopyOnWriteArrayList<>(isolationBuffer.flags); final @Nullable CopyOnWriteArrayList currentFlags = - currentBuffer == null ? null : currentBuffer.flags; + currentBuffer == null ? null : new CopyOnWriteArrayList<>(currentBuffer.flags); final int globalSize = globalFlags == null ? 0 : globalFlags.size(); final int isolationSize = isolationFlags == null ? 0 : isolationFlags.size(); diff --git a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt index 8ec18ce02b8..69ccbc9841b 100644 --- a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt +++ b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt @@ -1,6 +1,12 @@ package io.sentry.featureflags +import com.google.common.truth.Truth.assertThat import io.sentry.SentryOptions +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -347,4 +353,48 @@ class FeatureFlagBufferTest { val featureFlags = buffer.featureFlags assertNotNull(featureFlags) } + + @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) + + repeat(options.maxFeatureFlags) { globalBuffer.add("initial$it", true) } + + val stop = AtomicBoolean(false) + val writerFailure = AtomicReference(null) + val writerOperations = AtomicInteger() + val writerStarted = CountDownLatch(1) + val writer = Thread { + try { + var i = 0 + while (!stop.get()) { + globalBuffer.add("flag${i++ % 150}", true) + writerOperations.incrementAndGet() + writerStarted.countDown() + } + } catch (e: Exception) { + writerFailure.set(e) + } + } + + writer.start() + try { + assertThat(writerStarted.await(5, TimeUnit.SECONDS)).isTrue() + val writerOperationsBeforeMerging = writerOperations.get() + + repeat(1_000) { + FeatureFlagBuffer.merged(options, globalBuffer, isolationBuffer, currentBuffer).featureFlags + } + + assertThat(writerOperations.get()).isGreaterThan(writerOperationsBeforeMerging) + } finally { + stop.set(true) + writer.join() + } + + assertThat(writerFailure.get()).isNull() + } }