diff --git a/CHANGELOG.md b/CHANGELOG.md index f9c5dd4e6d6..a8cdbc9bf6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ - Remove internal API status from get/setDistinctId ([#4708](https://github.com/getsentry/sentry-java/pull/4708)) +### Fixes + +- Fix `NoSuchElementException` in `BufferCaptureStrategy` ([#4717](https://github.com/getsentry/sentry-java/pull/4717)) + ### Dependencies - Bump Native SDK from v0.10.0 to v0.10.1 ([#4695](https://github.com/getsentry/sentry-java/pull/4695)) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt index 4c3e348153c..72d8b7f29fb 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt @@ -42,11 +42,11 @@ public class ReplayCache(private val options: SentryOptions, private val replayI private val isClosed = AtomicBoolean(false) private val encoderLock = AutoClosableReentrantLock() private val lock = AutoClosableReentrantLock() + private val framesLock = AutoClosableReentrantLock() private var encoder: SimpleVideoEncoder? = null internal val replayCacheDir: File? by lazy { makeReplayCacheDir(options, replayId) } - // TODO: maybe account for multi-threaded access internal val frames = mutableListOf() private val ongoingSegment = LinkedHashMap() @@ -98,9 +98,13 @@ public class ReplayCache(private val options: SentryOptions, private val replayI */ public fun addFrame(screenshot: File, frameTimestamp: Long, screen: String? = null) { val frame = ReplayFrame(screenshot, frameTimestamp, screen) - frames += frame + framesLock.acquire().use { frames += frame } } + /** Returns the timestamp of the first frame if available in a thread-safe manner. */ + internal fun firstFrameTimestamp(): Long? = + framesLock.acquire().use { frames.firstOrNull()?.timestamp } + /** * Creates a video out of currently stored [frames] given the start time and duration using the * on-device codecs [android.media.MediaCodec]. The generated video will be stored in [videoFile] @@ -134,7 +138,10 @@ public class ReplayCache(private val options: SentryOptions, private val replayI if (videoFile.exists() && videoFile.length() > 0) { videoFile.delete() } - if (frames.isEmpty()) { + // Work on a snapshot of frames to avoid races with writers + val framesSnapshot = + framesLock.acquire().use { if (frames.isEmpty()) mutableListOf() else frames.toMutableList() } + if (framesSnapshot.isEmpty()) { options.logger.log(DEBUG, "No captured frames, skipping generating a video segment") return null } @@ -156,9 +163,9 @@ public class ReplayCache(private val options: SentryOptions, private val replayI val step = 1000 / frameRate.toLong() var frameCount = 0 - var lastFrame: ReplayFrame? = frames.first() + var lastFrame: ReplayFrame? = framesSnapshot.firstOrNull() for (timestamp in from until (from + (duration)) step step) { - val iter = frames.iterator() + val iter = framesSnapshot.iterator() while (iter.hasNext()) { val frame = iter.next() if (frame.timestamp in (timestamp..timestamp + step)) { @@ -180,7 +187,8 @@ public class ReplayCache(private val options: SentryOptions, private val replayI // if we failed to encode the frame, we delete the screenshot right away as the // likelihood of it being able to be encoded later is low deleteFile(lastFrame.screenshot) - frames.remove(lastFrame) + framesLock.acquire().use { frames.remove(lastFrame) } + framesSnapshot.remove(lastFrame) lastFrame = null } } @@ -240,14 +248,16 @@ public class ReplayCache(private val options: SentryOptions, private val replayI */ internal fun rotate(until: Long): String? { var screen: String? = null - frames.removeAll { - if (it.timestamp < until) { - deleteFile(it.screenshot) - return@removeAll true - } else if (screen == null) { - screen = it.screen + framesLock.acquire().use { + frames.removeAll { + if (it.timestamp < until) { + deleteFile(it.screenshot) + return@removeAll true + } else if (screen == null) { + screen = it.screen + } + return@removeAll false } - return@removeAll false } return screen } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index bdb3be62237..68ce255f02f 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -217,12 +217,10 @@ internal class BufferCaptureStrategy( val errorReplayDuration = options.sessionReplay.errorReplayDuration val now = dateProvider.currentTimeMillis val currentSegmentTimestamp = - if (cache?.frames?.isNotEmpty() == true) { + cache?.firstFrameTimestamp()?.let { // in buffer mode we have to set the timestamp of the first frame as the actual start - DateUtils.getDateTime(cache!!.frames.first().timestamp) - } else { - DateUtils.getDateTime(now - errorReplayDuration) - } + DateUtils.getDateTime(it) + } ?: DateUtils.getDateTime(now - errorReplayDuration) val duration = now - currentSegmentTimestamp.time val replayId = currentReplayId diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt index 6640cbf9faa..cc007d07067 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt @@ -98,7 +98,7 @@ internal class SessionCaptureStrategy( } if (currentConfig == null) { - options.logger.log(DEBUG, "Recorder config is not set, not recording frame") + options.logger.log(DEBUG, "Recorder config is not set, not capturing a segment") return@submitSafely } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt index 452dd343fd1..257941a9114 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt @@ -22,6 +22,8 @@ import io.sentry.rrweb.RRWebInteractionEvent import io.sentry.rrweb.RRWebInteractionEvent.InteractionType.TouchEnd import io.sentry.rrweb.RRWebInteractionEvent.InteractionType.TouchStart import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.atomic.AtomicReference import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals @@ -493,4 +495,106 @@ class ReplayCacheTest { assertTrue(replayCache.frames.isEmpty()) assertTrue(replayCache.replayCacheDir!!.listFiles()!!.none { it.extension == "jpg" }) } + + @Test + fun `firstFrameTimestamp returns first timestamp when available`() { + val replayCache = fixture.getSut(tmpDir) + + assertNull(replayCache.firstFrameTimestamp()) + + val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) + replayCache.addFrame(bitmap, 42) + replayCache.addFrame(bitmap, 1001) + + assertEquals(42L, replayCache.firstFrameTimestamp()) + } + + @Test + fun `firstFrameTimestamp is safe under concurrent rotate and add`() { + val replayCache = fixture.getSut(tmpDir) + + val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) + repeat(10) { i -> replayCache.addFrame(bitmap, (i + 1).toLong()) } + + val start = CountDownLatch(1) + val done = CountDownLatch(2) + val error = AtomicReference() + + val tReader = Thread { + try { + start.await() + repeat(500) { + replayCache.firstFrameTimestamp() + Thread.yield() + } + } catch (t: Throwable) { + error.set(t) + } finally { + done.countDown() + } + } + + val tWriter = Thread { + try { + start.await() + repeat(500) { i -> + if (i % 2 == 0) { + // delete all frames occasionally + replayCache.rotate(Long.MAX_VALUE) + } else { + // add a fresh frame + replayCache.addFrame(bitmap, System.currentTimeMillis()) + } + } + } catch (t: Throwable) { + error.set(t) + } finally { + done.countDown() + } + } + + tReader.start() + tWriter.start() + start.countDown() + done.await() + + // No crash is success + assertNull(error.get()) + } + + @Test + fun `createVideoOf tolerates concurrent rotate without crashing`() { + ReplayShadowMediaCodec.framesToEncode = 3 + val replayCache = fixture.getSut(tmpDir) + + val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) + // prepare a few frames that might be deleted during encoding + replayCache.addFrame(bitmap, 1) + replayCache.addFrame(bitmap, 1001) + replayCache.addFrame(bitmap, 2001) + + val start = CountDownLatch(1) + val done = CountDownLatch(1) + val error = AtomicReference() + + val tEncoder = Thread { + try { + start.await() + replayCache.createVideoOf(3000L, 0L, 0, 100, 200, 1, 20_000) + } catch (t: Throwable) { + error.set(t) + } finally { + done.countDown() + } + } + + tEncoder.start() + start.countDown() + // rotate while encoding to simulate concurrent mutation + replayCache.rotate(Long.MAX_VALUE) + done.await() + + // No crash is success + assertNull(error.get()) + } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt index e0aa07a77c7..b3fb9058a95 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt @@ -254,6 +254,74 @@ class BufferCaptureStrategyTest { assertEquals(ReplayType.BUFFER, converted.replayType) } + @Test + fun `createCurrentSegment uses first frame timestamp when available`() { + val now = System.currentTimeMillis() + val strategy = fixture.getSut(dateProvider = { now }) + strategy.start() + strategy.onConfigurationChanged(fixture.recorderConfig) + + // Stub first frame timestamp and capture the 'from' argument to createVideoOf + whenever(fixture.replayCache.firstFrameTimestamp()).thenReturn(1234L) + + var capturedFrom: Long = -1 + whenever( + fixture.replayCache.createVideoOf( + anyLong(), + anyLong(), + anyInt(), + anyInt(), + anyInt(), + anyInt(), + anyInt(), + any(), + ) + ) + .thenAnswer { invocation -> + capturedFrom = invocation.arguments[1] as Long + GeneratedVideo(File("0.mp4"), 5, VIDEO_DURATION) + } + + strategy.pause() + + assertEquals(1234L, capturedFrom) + assertEquals(1, strategy.currentSegment) + } + + @Test + fun `createCurrentSegment falls back to buffer start when no frames`() { + val now = System.currentTimeMillis() + val strategy = fixture.getSut(dateProvider = { now }) + strategy.start() + strategy.onConfigurationChanged(fixture.recorderConfig) + + // No frames available + whenever(fixture.replayCache.firstFrameTimestamp()).thenReturn(null) + + var capturedFrom: Long = -1 + whenever( + fixture.replayCache.createVideoOf( + anyLong(), + anyLong(), + anyInt(), + anyInt(), + anyInt(), + anyInt(), + anyInt(), + any(), + ) + ) + .thenAnswer { invocation -> + capturedFrom = invocation.arguments[1] as Long + GeneratedVideo(File("0.mp4"), 5, VIDEO_DURATION) + } + + strategy.pause() + + assertEquals(now - fixture.options.sessionReplay.errorReplayDuration, capturedFrom) + assertEquals(1, strategy.currentSegment) + } + @Test fun `captureReplay does not replayId to scope when not sampled`() { val strategy = fixture.getSut(onErrorSampleRate = 0.0)