diff --git a/CHANGELOG.md b/CHANGELOG.md index 247a5e96d31..8a1c2d02e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## Unreleased + +### Fixes + +- 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)) + +### Performance + +- Defer starting Session Replay off the SDK initialization critical path ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) + ## 8.53.0 ### Features diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api index 3efee26e37d..0e4ce0461b0 100644 --- a/sentry-android-replay/api/sentry-android-replay.api +++ b/sentry-android-replay/api/sentry-android-replay.api @@ -58,7 +58,7 @@ public final class io/sentry/android/replay/ReplayIntegration : io/sentry/IConne public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;)V public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;)V public synthetic fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;ILkotlin/jvm/internal/DefaultConstructorMarker;)V - public fun captureReplay (Ljava/lang/Boolean;)V + public fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public fun close ()V public fun disableDebugMaskingOverlay ()V public fun enableDebugMaskingOverlay ()V 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 92d4a0c4018..541d6a3b439 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 @@ -22,7 +22,6 @@ import java.io.File import java.io.StringReader import java.util.Date import java.util.LinkedList -import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean /** @@ -41,7 +40,6 @@ import java.util.concurrent.atomic.AtomicBoolean public class ReplayCache(private val options: SentryOptions, private val replayId: SentryId) : Closeable { private val isClosed = AtomicBoolean(false) - private val encoderLock = AutoClosableReentrantLock() private val lock = AutoClosableReentrantLock() private val framesLock = AutoClosableReentrantLock() private var encoder: SimpleVideoEncoder? = null @@ -152,28 +150,26 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } encoder = - encoderLock.acquire().use { - SimpleVideoEncoder( - options, - MuxerConfig( - file = videoFile, - recordingHeight = height, - recordingWidth = width, - frameRate = frameRate, - bitRate = bitRate, - ), - ) - .apply { - // the constructor already opened the MediaMuxer, so release it if start() fails, - // otherwise the encoder is never assigned and its resources leak (CloseGuard warning) - try { - start() - } catch (t: Throwable) { - release() - throw t - } + SimpleVideoEncoder( + options, + MuxerConfig( + file = videoFile, + recordingHeight = height, + recordingWidth = width, + frameRate = frameRate, + bitRate = bitRate, + ), + ) + .apply { + // the constructor already opened the MediaMuxer, so release it if start() fails, + // otherwise the encoder is never assigned and its resources leak (CloseGuard warning) + try { + start() + } catch (t: Throwable) { + release() + throw t } - } + } val step = 1000 / frameRate.toLong() var frameCount = 0 @@ -209,20 +205,15 @@ public class ReplayCache(private val options: SentryOptions, private val replayI if (frameCount == 0) { options.logger.log(DEBUG, "Generated a video with no frames, not capturing a replay segment") - encoderLock.acquire().use { - encoder?.release() - encoder = null - } + encoder?.release() + encoder = null deleteFile(videoFile) return null } - var videoDuration: Long - encoderLock.acquire().use { - encoder?.release() - videoDuration = encoder?.duration ?: 0 - encoder = null - } + encoder?.release() + val videoDuration = encoder?.duration ?: 0 + encoder = null rotate(until = (from + duration)) @@ -235,7 +226,7 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } return try { val bitmap = BitmapFactory.decodeFile(frame.screenshot.absolutePath) - encoderLock.acquire().use { encoder?.encode(bitmap) } + encoder?.encode(bitmap) bitmap.recycle() true } catch (e: Throwable) { @@ -281,27 +272,10 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } override fun close() { - // close() is called inline from the lifecycle path (ReplayIntegration.stop/close), which holds - // its own lock, so blocking here can freeze the main thread. If the encoder is wedged in a - // native MediaCodec call we'd never get the lock, so we give up instead: the already-dead codec - // is not released (leaking a native handle), which beats an ANR. try { - val token = encoderLock.tryAcquire(ENCODER_RELEASE_TIMEOUT_MS, MILLISECONDS) - if (token == null) { - options.logger.log( - WARNING, - "Timed out waiting for the video encoder, skipping its release to not block the caller", - ) - } else { - token.use { - encoder?.release() - encoder = null - } - } - } catch (e: InterruptedException) { - Thread.currentThread().interrupt() + encoder?.release() + encoder = null } finally { - // has to happen on all paths, callers rely on it to stop persisting segment values isClosed.set(true) } } @@ -333,13 +307,6 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } internal companion object { - /** - * How long [close] waits for the video encoder to become available. Below Android's ~5s ANR - * budget, and above the encoder's own bail-out (see MAX_EOS_STALL_ITERATIONS), so an encoder - * that's merely slow is still awaited rather than abandoned. - */ - private const val ENCODER_RELEASE_TIMEOUT_MS = 2000L - internal const val ONGOING_SEGMENT = ".ongoing_segment" internal const val SEGMENT_KEY_HEIGHT = "config.height" diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt index 98333260c7d..b9b17e12c8c 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt @@ -24,11 +24,11 @@ import io.sentry.SentryLevel.ERROR import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions import io.sentry.TypeCheckHint -import io.sentry.android.replay.ReplayState.CLOSED -import io.sentry.android.replay.ReplayState.PAUSED -import io.sentry.android.replay.ReplayState.RESUMED -import io.sentry.android.replay.ReplayState.STARTED -import io.sentry.android.replay.ReplayState.STOPPED +import io.sentry.android.replay.ReplayLifecycleState.CLOSED +import io.sentry.android.replay.ReplayLifecycleState.PAUSED +import io.sentry.android.replay.ReplayLifecycleState.RESUMED +import io.sentry.android.replay.ReplayLifecycleState.STARTED +import io.sentry.android.replay.ReplayLifecycleState.STOPPED import io.sentry.android.replay.capture.BufferCaptureStrategy import io.sentry.android.replay.capture.CaptureStrategy import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment @@ -47,7 +47,6 @@ import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter import io.sentry.transport.RateLimiter.IRateLimitObserver -import io.sentry.util.AutoClosableReentrantLock import io.sentry.util.FileUtils import io.sentry.util.HintUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion @@ -55,9 +54,12 @@ import io.sentry.util.Random import java.io.Closeable import java.io.File import java.util.LinkedList +import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.ThreadFactory +import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference public class ReplayIntegration( private val context: Context, @@ -100,13 +102,13 @@ public class ReplayIntegration( this.gestureRecorderProvider = gestureRecorderProvider } - @Volatile private var lastKnownConnectionStatus: ConnectionStatus = ConnectionStatus.UNKNOWN + private var lastKnownConnectionStatus: ConnectionStatus = ConnectionStatus.UNKNOWN private var debugMaskingEnabled: Boolean = false private lateinit var options: SentryOptions private var scopes: IScopes? = null private var recorder: Recorder? = null private var gestureRecorder: GestureRecorder? = null - private val random by lazy { Random() } + private val random = ThreadLocal() internal val rootViewsSpy by lazy { RootViewsSpy.install() } internal val lazyReplayExecutor = lazy { val delegate = Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory()) @@ -121,18 +123,16 @@ public class ReplayIntegration( internal val persistingExecutor by lazyPersistingExecutor internal val isEnabled = AtomicBoolean(false) - internal val isManualPause = AtomicBoolean(false) - private var captureStrategy: CaptureStrategy? = null + internal var isManualPause = false public val replayCacheDir: File? - get() = captureStrategy?.replayCacheDir + get() = state.get().captureStrategy?.replayCacheDir private var replayBreadcrumbConverter: ReplayBreadcrumbConverter = NoOpReplayBreadcrumbConverter.getInstance() private var replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null private var mainLooperHandler: MainLooperHandler = MainLooperHandler() private var gestureRecorderProvider: (() -> GestureRecorder)? = null - internal val lifecycleLock = AutoClosableReentrantLock() - private val lifecycle = ReplayLifecycle() + private val state = AtomicReference(ReplayState()) override fun register(scopes: IScopes, options: SentryOptions) { this.options = options @@ -165,110 +165,170 @@ public class ReplayIntegration( finalizePreviousReplay() } - override fun isRecording(): Boolean = - lifecycle.currentState >= STARTED && lifecycle.currentState < STOPPED + override fun isRecording(): Boolean = state.get().isRecording override fun start() { - lifecycleLock.acquire().use { - if (!isEnabled.get()) { - return - } - - if (!lifecycle.isAllowed(STARTED)) { - options.logger.log( - DEBUG, - "Session replay is already being recorded, not starting a new one", - ) - return - } + enqueueOnMainThread { startInternal() } + } - val isFullSession = random.sample(options.sessionReplay.sessionSampleRate) - if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) { - options.logger.log( - INFO, - "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified", - ) - return - } + private fun startInternal() { + if (!isEnabled.get()) { + return + } - lifecycle.currentState = STARTED - captureStrategy = - replayCaptureStrategyProvider?.invoke(isFullSession) - ?: if (isFullSession) { - SessionCaptureStrategy( - options, - scopes, - dateProvider, - replayExecutor, - persistingExecutor, - replayCacheProvider, - ) - } else { - BufferCaptureStrategy( - options, - scopes, - dateProvider, - random, - replayExecutor, - persistingExecutor, - replayCacheProvider, - ) - } - recorder?.start() - captureStrategy?.start() + val current = state.get() + if (!current.lifecycleState.isAllowed(STARTED)) { + options.logger.log( + DEBUG, + "Session replay is already being recorded, not starting a new one", + ) + return + } - registerRootViewListeners() + val isFullSession = sample(options.sessionReplay.sessionSampleRate) + if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) { + options.logger.log( + INFO, + "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified", + ) + return } + + val strategy = + replayCaptureStrategyProvider?.invoke(isFullSession) + ?: if (isFullSession) { + SessionCaptureStrategy( + options, + scopes, + dateProvider, + replayExecutor, + persistingExecutor, + replayCacheProvider, + ) + } else { + BufferCaptureStrategy( + options, + scopes, + dateProvider, + replayExecutor, + persistingExecutor, + replayCacheProvider, + ) + } + recorder?.start() + strategy.start() + val replayId: SentryId? = strategy.currentReplayId + state.set( + ReplayState( + generation = current.generation + 1, + lifecycleState = STARTED, + replayId = replayId ?: SentryId.EMPTY_ID, + captureStrategy = strategy, + ) + ) + + registerRootViewListeners() } override fun resume() { - isManualPause.set(false) - resumeInternal() + enqueueOnMainThread { + isManualPause = false + resumeInternal() + } } private fun resumeInternal() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) { - return - } - - if ( - isManualPause.get() || - lastKnownConnectionStatus == DISCONNECTED || - scopes?.rateLimiter?.isActiveForCategory(All) == true || - scopes?.rateLimiter?.isActiveForCategory(Replay) == true - ) { - return - } + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(RESUMED)) { + return + } - lifecycle.currentState = RESUMED - captureStrategy?.resume() - recorder?.resume() + if ( + isManualPause || + lastKnownConnectionStatus == DISCONNECTED || + scopes?.rateLimiter?.isActiveForCategory(All) == true || + scopes?.rateLimiter?.isActiveForCategory(Replay) == true + ) { + return } + + current.captureStrategy?.resume() + recorder?.resume() + state.set(current.copy(lifecycleState = RESUMED)) } - override fun captureReplay(isTerminating: Boolean?) { - if (!isEnabled.get() || !isRecording()) { - return + override fun captureReplay(isTerminating: Boolean?): SentryId { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { + return SentryId.EMPTY_ID } - if (SentryId.EMPTY_ID.equals(captureStrategy?.currentReplayId)) { + if (current.replayId == SentryId.EMPTY_ID) { options.logger.log(DEBUG, "Replay id is not set, not capturing for event") + return SentryId.EMPTY_ID + } + + if (current.isBuffering && !sample(options.sessionReplay.onErrorSampleRate)) { + options.logger.log( + INFO, + "Replay wasn't sampled by onErrorSampleRate, not capturing for event", + ) + return SentryId.EMPTY_ID + } + + // Set it synchronously so the event that triggered the flush picks it up before conversion. + scopes?.configureScope { it.replayId = current.replayId } + enqueueOnMainThread { + captureReplayInternal(current.generation, current.replayId, isTerminating == true) + } + return current.replayId + } + + private fun captureReplayInternal( + expectedGeneration: Long, + expectedReplayId: SentryId, + isTerminating: Boolean, + ) { + val current = state.get() + val strategy = current.captureStrategy + if (!current.matches(expectedGeneration, expectedReplayId) || strategy == null) { + options.logger.log( + INFO, + "Replay was stopped or restarted before capture could run, not capturing for event", + ) return } - captureStrategy?.captureReplay( - isTerminating == true, + var activeStrategy: CaptureStrategy = strategy + strategy.captureReplay( + isTerminating, onSegmentSent = { newTimestamp -> - captureStrategy?.currentSegment = captureStrategy?.currentSegment!! + 1 - captureStrategy?.segmentTimestamp = newTimestamp - captureStrategy?.isFlushed = true + enqueueOnMainThread { + val latest = state.get() + // The flush completes asynchronously; ignore it if this replay was stopped, restarted, + // or handed to another strategy in the meantime. + if ( + latest.matches(expectedGeneration, expectedReplayId) && + latest.captureStrategy === activeStrategy + ) { + activeStrategy.currentSegment++ + activeStrategy.segmentTimestamp = newTimestamp + activeStrategy.isFlushed = true + } + } }, ) - captureStrategy = captureStrategy?.convert() + activeStrategy = strategy.convert() + val replayId: SentryId? = activeStrategy.currentReplayId + state.set( + current.copy( + replayId = replayId ?: SentryId.EMPTY_ID, + captureStrategy = activeStrategy, + ) + ) } - override fun getReplayId(): SentryId = captureStrategy?.currentReplayId ?: SentryId.EMPTY_ID + override fun getReplayId(): SentryId = state.get().replayId override fun setBreadcrumbConverter(converter: ReplayBreadcrumbConverter) { replayBreadcrumbConverter = converter @@ -277,8 +337,10 @@ public class ReplayIntegration( override fun getBreadcrumbConverter(): ReplayBreadcrumbConverter = replayBreadcrumbConverter override fun pause() { - isManualPause.set(true) - pauseInternal() + enqueueOnMainThread { + isManualPause = true + pauseInternal() + } } override fun enableDebugMaskingOverlay() { @@ -292,51 +354,60 @@ public class ReplayIntegration( override fun isDebugMaskingOverlayEnabled(): Boolean = debugMaskingEnabled override fun registerTraceId(traceId: SentryId) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.registerTraceId(traceId) + current.captureStrategy?.registerTraceId(traceId) } override fun registerSegmentName(segmentName: String) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.registerSegmentName(segmentName) + current.captureStrategy?.registerSegmentName(segmentName) } private fun pauseInternal() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) { - return - } - - recorder?.pause() - captureStrategy?.pause() - lifecycle.currentState = PAUSED + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(PAUSED)) { + return } + + recorder?.pause() + current.captureStrategy?.pause() + state.set(current.copy(lifecycleState = PAUSED)) } override fun stop() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) { - return - } + enqueueOnMainThread { stopInternal() } + } - unregisterRootViewListeners() - recorder?.reset() - recorder?.stop() - gestureRecorder?.stop() - captureStrategy?.stop() - captureStrategy = null - lifecycle.currentState = STOPPED + private fun stopInternal() { + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(STOPPED)) { + return } + + unregisterRootViewListeners() + recorder?.reset() + recorder?.stop() + gestureRecorder?.stop() + current.captureStrategy?.stop() + state.set( + current.copy( + lifecycleState = STOPPED, + replayId = SentryId.EMPTY_ID, + captureStrategy = null, + ) + ) } override fun onScreenshotRecorded(bitmap: Bitmap) { var screen: String? = null scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } - captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> + state.get().captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> val observer = options.sessionReplay.frameObserver if (observer != null) { val copy = bitmap.copy(bitmap.config!!, false) @@ -353,13 +424,13 @@ public class ReplayIntegration( } addFrame(bitmap, frameTimeStamp, screen) } - postOnMainThread { checkCanRecord() } + enqueueOnMainThread { checkCanRecord() } } override fun onScreenshotRecorded(screenshot: File, frameTimestamp: Long) { var screen: String? = null scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } - captureStrategy?.onScreenshotRecorded { _ -> + state.get().captureStrategy?.onScreenshotRecorded { _ -> val observer = options.sessionReplay.frameObserver if (observer != null) { val bitmap = BitmapFactory.decodeFile(screenshot.absolutePath) @@ -376,34 +447,45 @@ public class ReplayIntegration( } addFrame(screenshot, frameTimestamp, screen) } - postOnMainThread { checkCanRecord() } + enqueueOnMainThread { checkCanRecord() } } override fun close() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(CLOSED)) { - return - } + if (!isEnabled.get()) { + return + } - options.connectionStatusProvider.removeConnectionStatusObserver(this) - scopes?.rateLimiter?.removeRateLimitObserver(this) - stop() - recorder?.close() - recorder = null - rootViewsSpy.close() - lifecycle.currentState = CLOSED + val isMainThread = Looper.myLooper() == Looper.getMainLooper() + val closeCompleted = if (isMainThread) null else CountDownLatch(1) + if (isMainThread) { + closeInternal() + } else { + mainLooperHandler.post { + try { + closeInternal() + } finally { + closeCompleted?.countDown() + } + } + } + if (closeCompleted != null) { + // Wait until main-thread teardown queues replay cleanup before shutting down its executors. + try { + closeCompleted.await(options.shutdownTimeoutMillis, MILLISECONDS) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + } } - // shutdown outside lock — awaiting termination while holding lifecycleLock deadlocks - // if any executor task tries to acquire the same lock + if (lazyReplayExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { + if (isMainThread) { replayExecutor.gracefulShutdown() } else { replayExecutor.shutdown() } } if (lazyPersistingExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { + if (isMainThread) { persistingExecutor.gracefulShutdown() } else { persistingExecutor.shutdown() @@ -411,51 +493,63 @@ public class ReplayIntegration( } } - override fun onConnectionStatusChanged(status: ConnectionStatus) { - lastKnownConnectionStatus = status - - if (captureStrategy !is SessionCaptureStrategy) { - // we only want to stop recording when offline for session mode + private fun closeInternal() { + if (!state.get().lifecycleState.isAllowed(CLOSED)) { return } - if (status == DISCONNECTED) { - pauseInternal() - } else { - // being positive for other states, even if it's NO_PERMISSION - resumeInternal() + options.connectionStatusProvider.removeConnectionStatusObserver(this) + scopes?.rateLimiter?.removeRateLimitObserver(this) + stopInternal() + recorder?.close() + recorder = null + rootViewsSpy.close() + state.set(state.get().copy(lifecycleState = CLOSED)) + } + + override fun onConnectionStatusChanged(status: ConnectionStatus) { + enqueueOnMainThread { + lastKnownConnectionStatus = status + if (state.get().captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when offline for session mode + return@enqueueOnMainThread + } + + if (status == DISCONNECTED) { + pauseInternal() + } else { + // being positive for other states, even if it's NO_PERMISSION + resumeInternal() + } } } override fun onRateLimitChanged(rateLimiter: RateLimiter) { - if (captureStrategy !is SessionCaptureStrategy) { - // we only want to stop recording when rate-limited for session mode - return - } + enqueueOnMainThread { + if (state.get().captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when rate-limited for session mode + return@enqueueOnMainThread + } - if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { - pauseInternal() - } else { - resumeInternal() + if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { + pauseInternal() + } else { + resumeInternal() + } } } override fun onTouchEvent(event: MotionEvent) { - if (!isEnabled.get() || !lifecycle.isTouchRecordingAllowed()) { + val current = state.get() + if (!isEnabled.get() || !current.isTouchRecordingAllowed) { return } - captureStrategy?.onTouchEvent(event) + current.captureStrategy?.onTouchEvent(event) } - // Runs [block] on the main thread. If already there, executes inline; otherwise posts via - // the main looper handler. Prevents deadlocks when lifecycle-lock-acquiring code (e.g. - // checkCanRecord -> pauseInternal) is called from the replay executor thread. - private inline fun postOnMainThread(crossinline block: () -> Unit) { - if (Looper.myLooper() == Looper.getMainLooper()) { - block() - } else { - mainLooperHandler.post { block() } - } + // Lifecycle commands are always queued so calls from main cannot overtake earlier commands. + private inline fun enqueueOnMainThread(crossinline block: () -> Unit) { + mainLooperHandler.post { block() } } /** @@ -464,7 +558,7 @@ public class ReplayIntegration( */ private fun checkCanRecord() { if ( - captureStrategy is SessionCaptureStrategy && + state.get().captureStrategy is SessionCaptureStrategy && (lastKnownConnectionStatus == DISCONNECTED || scopes?.rateLimiter?.isActiveForCategory(All) == true || scopes?.rateLimiter?.isActiveForCategory(Replay) == true) @@ -562,7 +656,7 @@ public class ReplayIntegration( } override fun onWindowSizeChanged(width: Int, height: Int) { - if (!isEnabled.get() || !isRecording()) { + if (!isEnabled.get() || !state.get().isRecording) { return } if (options.sessionReplay.isTrackConfiguration) { @@ -573,18 +667,41 @@ public class ReplayIntegration( } public fun onConfigurationChanged(config: ScreenshotRecorderConfig) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.onConfigurationChanged(config) + current.captureStrategy?.onConfigurationChanged(config) recorder?.onConfigurationChanged(config) // we have to restart recorder with a new config and pause immediately if the replay is paused - if (lifecycle.currentState == PAUSED) { + if (current.lifecycleState == PAUSED) { recorder?.pause() } } + private fun sample(rate: Double?): Boolean = + (random.get() ?: Random().also { random.set(it) }).sample(rate) + + private data class ReplayState( + val generation: Long = 0, + val lifecycleState: ReplayLifecycleState = ReplayLifecycleState.INITIAL, + val replayId: SentryId = SentryId.EMPTY_ID, + val captureStrategy: CaptureStrategy? = null, + ) { + val isBuffering: Boolean + get() = captureStrategy is BufferCaptureStrategy + + val isRecording: Boolean + get() = lifecycleState >= STARTED && lifecycleState < STOPPED + + val isTouchRecordingAllowed: Boolean + get() = lifecycleState == STARTED || lifecycleState == RESUMED + + fun matches(generation: Long, replayId: SentryId): Boolean = + isRecording && this.generation == generation && this.replayId == replayId + } + private class PreviousReplayHint : Backfillable { override fun shouldEnrich(): Boolean = false } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt index 38d0ae8bda8..a237e3ae30d 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt @@ -1,6 +1,6 @@ package io.sentry.android.replay -internal enum class ReplayState { +internal enum class ReplayLifecycleState { /** * Initial state of a Replay session. This is the state when ReplayIntegration is constructed but * has not been started yet. @@ -38,29 +38,23 @@ internal enum class ReplayState { CLOSED, } -/** Class to manage state transitions for ReplayIntegration */ -internal class ReplayLifecycle { - @field:Volatile internal var currentState = ReplayState.INITIAL - - fun isAllowed(newState: ReplayState): Boolean = - when (currentState) { - ReplayState.INITIAL -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED - ReplayState.STARTED -> - newState == ReplayState.PAUSED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.RESUMED -> - newState == ReplayState.PAUSED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.PAUSED -> - newState == ReplayState.RESUMED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.STOPPED -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED - ReplayState.CLOSED -> false - } - - fun isTouchRecordingAllowed(): Boolean = - currentState == ReplayState.STARTED || currentState == ReplayState.RESUMED -} +internal fun ReplayLifecycleState.isAllowed(newState: ReplayLifecycleState): Boolean = + when (this) { + ReplayLifecycleState.INITIAL -> + newState == ReplayLifecycleState.STARTED || newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.STARTED -> + newState == ReplayLifecycleState.PAUSED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.RESUMED -> + newState == ReplayLifecycleState.PAUSED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.PAUSED -> + newState == ReplayLifecycleState.RESUMED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.STOPPED -> + newState == ReplayLifecycleState.STARTED || newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.CLOSED -> false + } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt index f505d21a151..deb51ecc006 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt @@ -119,10 +119,15 @@ internal abstract class BaseCaptureStrategy( override fun pause() = Unit override fun stop() { - cache?.close() - replayStartTimestamp.set(0) - segmentTimestamp = null - currentReplayId = SentryId.EMPTY_ID + // Keep cleanup behind queued frames; a later start uses a new capture strategy instance. + replayExecutor.submit( + ReplayRunnable("$TAG.stop") { + cache?.close() + replayStartTimestamp.set(0) + segmentTimestamp = null + currentReplayId = SentryId.EMPTY_ID + } + ) } protected fun createSegmentInternal( 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 4d7bcd64cf4..d20735e4128 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 @@ -18,12 +18,10 @@ import io.sentry.android.replay.ScreenshotRecorderConfig import io.sentry.android.replay.capture.CaptureStrategy.Companion.rotateEvents import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment import io.sentry.android.replay.util.ReplayRunnable -import io.sentry.android.replay.util.sample import io.sentry.clientreport.DiscardReason.RATELIMIT_BACKOFF import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.util.FileUtils -import io.sentry.util.Random import java.io.File import java.util.Date import java.util.concurrent.ScheduledExecutorService @@ -46,7 +44,6 @@ internal class BufferCaptureStrategy( private val options: SentryOptions, private val scopes: IScopes?, private val dateProvider: ICurrentDateProvider, - private val random: Random, executor: ScheduledExecutorService, persistingExecutor: ScheduledExecutorService, replayCacheProvider: ((replayId: SentryId) -> ReplayCache)? = null, @@ -91,20 +88,6 @@ internal class BufferCaptureStrategy( } override fun captureReplay(isTerminating: Boolean, onSegmentSent: (Date) -> Unit) { - val sampled = random.sample(options.sessionReplay.onErrorSampleRate) - - if (!sampled) { - options.logger.log( - INFO, - "Replay wasn't sampled by onErrorSampleRate, not capturing for event", - ) - return - } - - // write replayId to scope right away, so it gets picked up by the event that caused buffer - // to flush - scopes?.configureScope { it.replayId = currentReplayId } - if (isTerminating) { this.isTerminating.set(true) // avoid capturing replay, because the video will be malformed 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 96e5a926af4..b3f3307837b 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 @@ -64,8 +64,6 @@ class ReplayCacheTest { ReplayShadowMediaCodec.framesToEncode = 5 ReplayShadowMediaCodec.throwOnStart = false ReplayShadowMediaCodec.neverSignalEos = false - ReplayShadowMediaCodec.blockOnDequeue = null - ReplayShadowMediaCodec.blockedOnDequeue = CountDownLatch(1) ReplayShadowMediaCodec.released = false ShadowBitmapFactory.setAllowInvalidImageData(true) } @@ -691,46 +689,6 @@ class ReplayCacheTest { assertThat(error.get()).isNull() } - @Test - fun `close does not block when the encoder is wedged, and still marks the cache closed`() { - val wedge = CountDownLatch(1) - ReplayShadowMediaCodec.blockOnDequeue = wedge - val replayCache = fixture.getSut(tmpDir) - - val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) - replayCache.addFrame(bitmap, 1) - - // parks inside MediaCodec while holding the encoder lock - val encoder = - thread(isDaemon = true) { replayCache.createVideoOf(1000L, 0L, 0, 100, 200, 1, 20_000) } - try { - assertWithMessage("the encoder never reached dequeueOutputBuffer") - .that(ReplayShadowMediaCodec.blockedOnDequeue.await(30, SECONDS)) - .isTrue() - - // on a separate thread so a regression fails the test instead of hanging the run - val closed = CountDownLatch(1) - thread(isDaemon = true) { - replayCache.close() - closed.countDown() - } - assertWithMessage("close() blocked on the wedged encoder") - .that(closed.await(30, SECONDS)) - .isTrue() - - // giving up on the lock still counts as closed, otherwise we'd keep persisting segments - replayCache.persistSegmentValues(SEGMENT_KEY_ID, "0") - assertThat(File(replayCache.replayCacheDir, ONGOING_SEGMENT).exists()).isFalse() - - assertWithMessage("encoder should not be released when the lock times out") - .that(ReplayShadowMediaCodec.released) - .isFalse() - } finally { - wedge.countDown() - encoder.join(SECONDS.toMillis(10)) - } - } - @Test fun `createVideoOf releases the encoder even when EOS is never signalled`() { ReplayShadowMediaCodec.neverSignalEos = true diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index 32b7f4e9285..394e1c00839 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -4,8 +4,10 @@ import android.content.Context import android.graphics.Bitmap import android.graphics.Bitmap.CompressFormat.JPEG import android.graphics.Bitmap.Config.ARGB_8888 +import android.os.Looper import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.Hint @@ -34,6 +36,7 @@ import io.sentry.android.replay.capture.CaptureStrategy import io.sentry.android.replay.capture.SessionCaptureStrategy import io.sentry.android.replay.capture.SessionCaptureStrategyTest.Fixture.Companion.VIDEO_DURATION import io.sentry.android.replay.gestures.GestureRecorder +import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.ReplayShadowMediaCodec import io.sentry.cache.PersistingScopeObserver import io.sentry.cache.tape.QueueFile @@ -47,14 +50,17 @@ import io.sentry.rrweb.RRWebVideoEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter -import io.sentry.util.Random import java.io.ByteArrayOutputStream import java.io.File +import java.util.Date +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue +import org.awaitility.kotlin.await import org.junit.Rule import org.junit.rules.TemporaryFolder import org.junit.runner.RunWith @@ -73,6 +79,7 @@ import org.mockito.kotlin.reset import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever +import org.robolectric.Shadows.shadowOf import org.robolectric.annotation.Config @RunWith(AndroidJUnit4::class) @@ -128,6 +135,14 @@ class ReplayIntegrationTest { replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null, gestureRecorderProvider: (() -> GestureRecorder)? = null, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), + mainLooperHandler: MainLooperHandler = mock { + doAnswer { + (it.arguments[0] as Runnable).run() + true + } + .whenever(mock) + .post(any()) + }, ): ReplayIntegration { options.run { sessionReplay.onErrorSampleRate = onErrorSampleRate @@ -142,6 +157,7 @@ class ReplayIntegrationTest { recorderProvider, replayCacheProvider = { _ -> replayCache }, replayCaptureStrategyProvider = replayCaptureStrategyProvider, + mainLooperHandler = mainLooperHandler, gestureRecorderProvider = gestureRecorderProvider, ) } @@ -215,6 +231,25 @@ class ReplayIntegrationTest { assertTrue(replay.isRecording) } + @Test + fun `start is deferred when called on main`() { + val captureStrategy = mock() + val replay = + fixture.getSut( + context, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + + replay.start() + + assertThat(replay.isRecording).isFalse() + verify(captureStrategy, never()).start(any(), any(), anyOrNull()) + shadowOf(Looper.getMainLooper()).idle() + assertThat(replay.isRecording).isTrue() + } + @Test fun `starting two times does nothing`() { val captureStrategy = mock() @@ -337,6 +372,7 @@ class ReplayIntegrationTest { fun `captureReplay calls and converts strategy`() { val captureStrategy = mock { whenever(mock.currentReplayId).thenReturn(SentryId()) } + whenever(captureStrategy.convert()).thenReturn(captureStrategy) val replay = fixture.getSut(context, replayCaptureStrategyProvider = { captureStrategy }) replay.register(fixture.scopes, fixture.options) @@ -352,6 +388,115 @@ class ReplayIntegrationTest { verify(captureStrategy).convert() } + @Test + fun `captureReplay returns replay id and sets scope before queued capture`() { + val replayId = SentryId() + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(replayId) + whenever(captureStrategy.convert()).thenReturn(captureStrategy) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + + val returnedReplayId = replay.captureReplay(false) + + assertThat(returnedReplayId).isEqualTo(replayId) + assertThat(fixture.scope.replayId).isEqualTo(replayId) + verify(captureStrategy, never()).captureReplay(any(), any()) + shadowOf(Looper.getMainLooper()).idle() + verify(captureStrategy).captureReplay(eq(false), any()) + } + + @Test + fun `captureReplay returns empty id when error replay is not sampled`() { + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(SentryId()) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + onErrorSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + + assertThat(replay.captureReplay(false)).isEqualTo(SentryId.EMPTY_ID) + verify(captureStrategy, never()).captureReplay(any(), any()) + } + + @Test + fun `capture queued after stop cannot resurrect replay`() { + val replayId = SentryId() + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(replayId) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + + replay.stop() + assertThat(replay.captureReplay(false)).isEqualTo(replayId) + shadowOf(Looper.getMainLooper()).idle() + + assertThat(replay.isRecording).isFalse() + assertThat(replay.replayId).isEqualTo(SentryId.EMPTY_ID) + verify(captureStrategy, never()).captureReplay(any(), any()) + } + + @Test + fun `stale capture callback cannot mutate restarted replay`() { + val oldReplayId = SentryId() + val newReplayId = SentryId() + var onSegmentSent: ((Date) -> Unit)? = null + val oldStrategy = mock() + whenever(oldStrategy.currentReplayId).thenReturn(oldReplayId) + whenever(oldStrategy.convert()).thenReturn(oldStrategy) + doAnswer { + @Suppress("UNCHECKED_CAST") + onSegmentSent = it.arguments[1] as (Date) -> Unit + } + .whenever(oldStrategy) + .captureReplay(any(), any()) + val newStrategy = mock() + whenever(newStrategy.currentReplayId).thenReturn(newReplayId) + whenever(newStrategy.currentSegment).thenThrow(AssertionError("stale callback")) + var starts = 0 + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { if (starts++ == 0) oldStrategy else newStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + replay.captureReplay(false) + shadowOf(Looper.getMainLooper()).idle() + + replay.stop() + replay.start() + shadowOf(Looper.getMainLooper()).idle() + onSegmentSent?.invoke(Date()) + shadowOf(Looper.getMainLooper()).idle() + + assertThat(replay.replayId).isEqualTo(newReplayId) + } + @Test fun `pause does nothing when not recording`() { val captureStrategy = mock() @@ -444,6 +589,90 @@ class ReplayIntegrationTest { assertFalse(replay.isRecording()) } + @Test + fun `background lifecycle calls run on main thread in order`() { + val calls = mutableListOf() + val captureStrategy = + mock { + doAnswer { calls += "start" }.whenever(mock).start(any(), any(), anyOrNull()) + doAnswer { calls += "pause" }.whenever(mock).pause() + doAnswer { calls += "resume" }.whenever(mock).resume() + doAnswer { calls += "stop" }.whenever(mock).stop() + } + val replay = + fixture.getSut( + context, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + + Thread { replay.start() } + .apply { + start() + join() + } + replay.pause() + replay.resume() + replay.stop() + + assertThat(calls).isEmpty() + shadowOf(Looper.getMainLooper()).idle() + assertThat(calls).containsExactly("start", "pause", "resume", "stop").inOrder() + } + + @Test + fun `background close waits for main thread teardown`() { + fixture.options.shutdownTimeoutMillis = TimeUnit.SECONDS.toMillis(30) + val recorder = mock() + val replay = + fixture.getSut( + context, + recorderProvider = { recorder }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + + val closeThread = Thread { replay.close() }.apply { start() } + await.until { closeThread.state == Thread.State.TIMED_WAITING } + + verify(recorder, never()).close() + shadowOf(Looper.getMainLooper()).idle() + closeThread.join(TimeUnit.SECONDS.toMillis(2)) + + assertThat(closeThread.isAlive).isFalse() + verify(recorder).close() + } + + @Test + fun `main thread close does not wait for replay executor`() { + val replay = fixture.getSut(context, replayCaptureStrategyProvider = { mock() }) + replay.register(fixture.scopes, fixture.options) + replay.start() + + val running = CountDownLatch(1) + val release = CountDownLatch(1) + val finished = CountDownLatch(1) + replay.replayExecutor.submit { + running.countDown() + try { + release.await() + } finally { + finished.countDown() + } + } + assertThat(running.await(10, TimeUnit.SECONDS)).isTrue() + + try { + replay.close() + assertThat(finished.count).isEqualTo(1L) + } finally { + release.countDown() + } + assertThat(finished.await(10, TimeUnit.SECONDS)).isTrue() + } + @Test fun `onConfigurationChanged does nothing when not recording`() { val captureStrategy = mock() @@ -746,7 +975,6 @@ class ReplayIntegrationTest { ICurrentDateProvider { System.currentTimeMillis() + fixture.options.sessionReplay.sessionSegmentDuration }, - Random(), // run tasks synchronously in tests mock { whenever(mock.submit(any())).doAnswer { diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt index 75626f4e4cf..44f1551ede2 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt @@ -33,6 +33,7 @@ import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -51,7 +52,22 @@ class ReplayIntegrationWithRecorderTest { context: Context, recorder: Recorder, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), - ): ReplayIntegration = ReplayIntegration(context, dateProvider, recorderProvider = { recorder }) + ): ReplayIntegration = + ReplayIntegration( + context, + dateProvider, + recorderProvider = { recorder }, + replayCacheProvider = null, + mainLooperHandler = + mock { + doAnswer { + (it.arguments[0] as Runnable).run() + true + } + .whenever(mock) + .post(any()) + }, + ) } private val fixture = Fixture() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt index 4b5e45d23d7..5bd897f9ec4 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt @@ -1,116 +1,67 @@ package io.sentry.android.replay import kotlin.test.Test -import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue class ReplayLifecycleTest { - @Test - fun `verify initial state`() { - val lifecycle = ReplayLifecycle() - assertEquals(ReplayState.INITIAL, lifecycle.currentState) - } - @Test fun `test transitions from INITIAL state`() { - val lifecycle = ReplayLifecycle() + assertTrue(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.STARTED)) + assertTrue(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STARTED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STOPPED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.STOPPED)) } @Test fun `test transitions from STARTED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.STARTED - - assertTrue(lifecycle.isAllowed(ReplayState.PAUSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.PAUSED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.CLOSED)) - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from RESUMED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.RESUMED + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.PAUSED)) + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.PAUSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from PAUSED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.PAUSED - - assertTrue(lifecycle.isAllowed(ReplayState.RESUMED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.RESUMED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.CLOSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from STOPPED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.STOPPED + assertTrue(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.STARTED)) + assertTrue(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STARTED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from CLOSED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.CLOSED - - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STOPPED)) - assertFalse(lifecycle.isAllowed(ReplayState.CLOSED)) - } - - @Test - fun `test touch recording is allowed only in STARTED and RESUMED states`() { - val lifecycle = ReplayLifecycle() - - // Initial state doesn't allow touch recording - assertFalse(lifecycle.isTouchRecordingAllowed()) - - // STARTED state allows touch recording - lifecycle.currentState = ReplayState.STARTED - assertTrue(lifecycle.isTouchRecordingAllowed()) - - // RESUMED state allows touch recording - lifecycle.currentState = ReplayState.RESUMED - assertTrue(lifecycle.isTouchRecordingAllowed()) - - // Other states don't allow touch recording - val otherStates = - listOf(ReplayState.INITIAL, ReplayState.PAUSED, ReplayState.STOPPED, ReplayState.CLOSED) - - otherStates.forEach { state -> - lifecycle.currentState = state - assertFalse(lifecycle.isTouchRecordingAllowed()) - } + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.INITIAL)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.STOPPED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.CLOSED)) } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt index b5e15b5534f..b84b1b53347 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt @@ -25,14 +25,12 @@ import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter import java.time.Duration -import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.BeforeTest import kotlin.test.assertEquals import kotlin.test.assertNotEquals -import kotlin.test.assertTrue import org.awaitility.core.ConditionTimeoutException import org.awaitility.kotlin.await import org.junit.Rule @@ -256,45 +254,6 @@ class ReplaySmokeTest { assertNotEquals(falseReplay.rootViewsSpy, replay.rootViewsSpy) assertEquals(0, falseReplay.rootViewsSpy.listeners.size) } - - @Test - fun `close does not deadlock when executor task is waiting on lifecycleLock`() { - fixture.options.sessionReplay.sessionSampleRate = 1.0 - fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath - - val replay = fixture.getSut(context) - replay.register(fixture.scopes, fixture.options) - replay.start() - - val taskBlocked = CountDownLatch(1) - val lockReleased = CountDownLatch(1) - - // hold lifecycleLock on this thread - val token = replay.lifecycleLock.acquire() - - // submit a task on the executor that tries to acquire the same lock — it will block - replay.replayExecutor.submit { - taskBlocked.countDown() - replay.lifecycleLock.acquire().use {} - } - - // wait for the executor task to actually be running and blocked - assertTrue(taskBlocked.await(2, TimeUnit.SECONDS)) - - // release the lock, then close — if shutdown were inside the lock this would deadlock - token.close() - - // close() must complete within a reasonable time - val closedInTime = AtomicBoolean(false) - val closeThread = Thread { - replay.close() - closedInTime.set(true) - } - closeThread.start() - closeThread.join(5000) - - assertTrue(closedInTime.get(), "close() deadlocked") - } } private class ExampleActivity : Activity() { 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 fc1981a84b1..0d12fd3c3ff 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 @@ -26,7 +26,6 @@ import io.sentry.protocol.SentryId import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter -import io.sentry.util.Random import java.io.File import kotlin.test.Test import kotlin.test.assertEquals @@ -110,17 +109,14 @@ class BufferCaptureStrategyTest { .orEmpty() fun getSut( - onErrorSampleRate: Double = 1.0, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), replayCacheDir: File? = null, ): BufferCaptureStrategy { replayCacheDir?.let { whenever(replayCache.replayCacheDir).thenReturn(it) } - options.run { sessionReplay.onErrorSampleRate = onErrorSampleRate } return BufferCaptureStrategy( options, scopes, dateProvider, - Random(), mock { whenever(it.submit(any())).doAnswer { invocation -> (invocation.arguments[0] as Runnable).run() @@ -355,16 +351,6 @@ class BufferCaptureStrategyTest { assertEquals(1, strategy.currentSegment) } - @Test - fun `captureReplay does not replayId to scope when not sampled`() { - val strategy = fixture.getSut(onErrorSampleRate = 0.0) - strategy.start() - - strategy.captureReplay(false) {} - - assertEquals(SentryId.EMPTY_ID, fixture.scope.replayId) - } - @Test fun `captureReplay does not capture segments when rate-limited`() { val rateLimiter = mock { on { isActiveForCategory(any()) }.thenReturn(true) } @@ -378,9 +364,6 @@ class BufferCaptureStrategyTest { // neither the current nor the buffered segment should be sent while rate-limited verify(fixture.scopes, never()).captureReplay(any(), any()) - // the replayId is still set on the scope so the error that flushed the buffer stays linked to - // the replay that gets recorded once the rate limit lifts - assertEquals(strategy.currentReplayId, fixture.scope.replayId) } @Test @@ -412,7 +395,7 @@ class BufferCaptureStrategyTest { } @Test - fun `captureReplay sets replayId to scope and captures buffered segments`() { + fun `captureReplay captures buffered segments`() { var called = false val strategy = fixture.getSut() strategy.start() @@ -424,7 +407,6 @@ class BufferCaptureStrategyTest { // buffered + current = 2 verify(fixture.scopes, times(2)).captureReplay(any(), any()) - assertEquals(strategy.currentReplayId, fixture.scope.replayId) assertTrue(called) } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index fc2354eb1c0..3e5198bea01 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -1,6 +1,7 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap +import com.google.common.truth.Truth.assertThat import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.IScopes @@ -33,6 +34,7 @@ import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import java.io.File import java.util.Date +import java.util.concurrent.ScheduledExecutorService import kotlin.test.Test import kotlin.test.assertContentEquals import kotlin.test.assertEquals @@ -108,20 +110,21 @@ class SessionCaptureStrategyTest { fun getSut( dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), replayCacheDir: File? = null, + replayExecutor: ScheduledExecutorService = mock { + doAnswer { invocation -> + (invocation.arguments[0] as Runnable).run() + null + } + .whenever(it) + .submit(any()) + }, ): SessionCaptureStrategy { replayCacheDir?.let { whenever(replayCache.replayCacheDir).thenReturn(it) } return SessionCaptureStrategy( options, scopes, dateProvider, - mock { - doAnswer { invocation -> - (invocation.arguments[0] as Runnable).run() - null - } - .whenever(it) - .submit(any()) - }, + replayExecutor, mock { doAnswer { invocation -> (invocation.arguments[0] as Runnable).run() @@ -213,6 +216,31 @@ class SessionCaptureStrategyTest { verify(fixture.replayCache).close() } + @Test + fun `stop closes cache after queued replay work`() { + val tasks = mutableListOf() + val calls = mutableListOf() + val replayExecutor = + mock { + doAnswer { + tasks += it.arguments[0] as Runnable + null + } + .whenever(mock) + .submit(any()) + } + doAnswer { calls += "close" }.whenever(fixture.replayCache).close() + val strategy = fixture.getSut(replayExecutor = replayExecutor) + strategy.start() + replayExecutor.submit(Runnable { calls += "encode" }) + + strategy.stop() + + verify(fixture.replayCache, never()).close() + tasks.forEach(Runnable::run) + assertThat(calls).containsExactly("encode", "close").inOrder() + } + @Test fun `captureReplay does nothing for non-crashed event`() { val strategy = fixture.getSut() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt index e0e13076ea0..114d9e5fd24 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt @@ -3,7 +3,6 @@ package io.sentry.android.replay.util import android.media.MediaCodec import android.media.MediaCodec.BufferInfo import java.nio.ByteBuffer -import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit.MICROSECONDS import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean @@ -21,15 +20,6 @@ class ReplayShadowMediaCodec : ShadowMediaCodec() { /** Simulates an encoder that never emits [MediaCodec.BUFFER_FLAG_END_OF_STREAM]. */ var neverSignalEos = false - /** - * When set, [dequeueOutputBuffer] awaits this latch, simulating a native call that never - * returns. [blockedOnDequeue] is counted down right before, so tests can wait until the codec - * is actually stuck. - */ - var blockOnDequeue: CountDownLatch? = null - - var blockedOnDequeue = CountDownLatch(1) - /** Set to `true` when [release] is called. */ var released = false } @@ -61,10 +51,6 @@ class ReplayShadowMediaCodec : ShadowMediaCodec() { @Implementation fun dequeueOutputBuffer(info: BufferInfo, timeoutUs: Long): Int { - blockOnDequeue?.let { - blockedOnDequeue.countDown() - it.await() - } val encoderStatus = super.native_dequeueOutputBuffer(info, timeoutUs) super.validateOutputByteBuffer(getOutputBuffers(), encoderStatus, info) if (encoderStatus == MediaCodec.INFO_TRY_AGAIN_LATER && !encoded.getAndSet(true)) { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 54729fdb12b..233a83b12b8 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1707,7 +1707,7 @@ public final class io/sentry/NoOpReplayBreadcrumbConverter : io/sentry/ReplayBre } public final class io/sentry/NoOpReplayController : io/sentry/ReplayController { - public fun captureReplay (Ljava/lang/Boolean;)V + public fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public fun disableDebugMaskingOverlay ()V public fun enableDebugMaskingOverlay ()V public fun getBreadcrumbConverter ()Lio/sentry/ReplayBreadcrumbConverter; @@ -2361,7 +2361,7 @@ public abstract interface class io/sentry/ReplayBreadcrumbConverter { } public abstract interface class io/sentry/ReplayController : io/sentry/IReplayApi { - public abstract fun captureReplay (Ljava/lang/Boolean;)V + public abstract fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public abstract fun getBreadcrumbConverter ()Lio/sentry/ReplayBreadcrumbConverter; public abstract fun getReplayId ()Lio/sentry/protocol/SentryId; public abstract fun isDebugMaskingOverlayEnabled ()Z @@ -7646,7 +7646,6 @@ public final class io/sentry/util/AutoClosableReentrantLock : io/sentry/ISentryL public fun ()V public fun acquire ()Lio/sentry/ISentryLifecycleToken; public fun close ()V - public fun tryAcquire (JLjava/util/concurrent/TimeUnit;)Lio/sentry/ISentryLifecycleToken; } public final class io/sentry/util/CheckInUtils { diff --git a/sentry/src/main/java/io/sentry/NoOpReplayController.java b/sentry/src/main/java/io/sentry/NoOpReplayController.java index 2b8a09cb1d9..3f1e88b822b 100644 --- a/sentry/src/main/java/io/sentry/NoOpReplayController.java +++ b/sentry/src/main/java/io/sentry/NoOpReplayController.java @@ -32,7 +32,9 @@ public boolean isRecording() { } @Override - public void captureReplay(@Nullable Boolean isTerminating) {} + public @NotNull SentryId captureReplay(@Nullable Boolean isTerminating) { + return SentryId.EMPTY_ID; + } @Override public @NotNull SentryId getReplayId() { diff --git a/sentry/src/main/java/io/sentry/ReplayController.java b/sentry/src/main/java/io/sentry/ReplayController.java index 2fb7b1c83a5..630c0da3d50 100644 --- a/sentry/src/main/java/io/sentry/ReplayController.java +++ b/sentry/src/main/java/io/sentry/ReplayController.java @@ -17,7 +17,12 @@ public interface ReplayController extends IReplayApi { boolean isRecording(); - void captureReplay(@Nullable Boolean isTerminating); + /** + * Captures the buffered replay and returns its ID, or {@link SentryId#EMPTY_ID} if no replay was + * captured. + */ + @NotNull + SentryId captureReplay(@Nullable Boolean isTerminating); @NotNull SentryId getReplayId(); diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 92037f6690b..9d2c4403194 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -254,16 +254,18 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul } } if (shouldCaptureReplay) { - options.getReplayController().captureReplay(event.isCrashed()); - if (scope != null) { - final @Nullable SentryId replayId = scope.getReplayId(); - if (replayId != null && !replayId.equals(SentryId.EMPTY_ID)) { - final @Nullable ITransaction transaction = scope.getTransaction(); - if (transaction != null) { - final @Nullable Baggage baggage = transaction.getSpanContext().getBaggage(); - if (baggage != null) { - baggage.forceSetReplayId(replayId); - } + final @NotNull SentryId scopeReplayId = + scope != null ? scope.getReplayId() : SentryId.EMPTY_ID; + final @NotNull SentryId capturedReplayId = + options.getReplayController().captureReplay(event.isCrashed()); + final @NotNull SentryId replayId = + !capturedReplayId.equals(SentryId.EMPTY_ID) ? capturedReplayId : scopeReplayId; + if (scope != null && !replayId.equals(SentryId.EMPTY_ID)) { + final @Nullable ITransaction transaction = scope.getTransaction(); + if (transaction != null) { + final @Nullable Baggage baggage = transaction.getSpanContext().getBaggage(); + if (baggage != null) { + baggage.forceSetReplayId(replayId); } } } @@ -1272,8 +1274,10 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint // If feedback already has a replayId, we don't want to overwrite it. if (feedback.getReplayId() == null) { - options.getReplayController().captureReplay(false); - final @NotNull SentryId replayId = scope.getReplayId(); + final @NotNull SentryId scopeReplayId = scope.getReplayId(); + final @NotNull SentryId capturedReplayId = options.getReplayController().captureReplay(false); + final @NotNull SentryId replayId = + !capturedReplayId.equals(SentryId.EMPTY_ID) ? capturedReplayId : scopeReplayId; if (!replayId.equals(SentryId.EMPTY_ID)) { feedback.setReplayId(replayId); } diff --git a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java index 617c1a5b4fa..cf53d860e08 100644 --- a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java +++ b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java @@ -1,7 +1,6 @@ package io.sentry.util; import io.sentry.ISentryLifecycleToken; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.concurrent.locks.ReentrantLock; import org.jetbrains.annotations.ApiStatus; @@ -39,19 +38,6 @@ public final class AutoClosableReentrantLock implements ISentryLifecycleToken { return this; } - /** - * Like {@link #acquire()}, but gives up after {@code timeout}. Use it when blocking forever would - * be worse than not doing the work at all, e.g. on a path that can run on the main thread. - * - * @return the token (this instance) if the lock was acquired, or {@code null} if it wasn't. A - * {@code null} return means the lock is not held, so {@link #close()} must not be - * called for it. - */ - public @Nullable ISentryLifecycleToken tryAcquire( - final long timeout, final @NotNull TimeUnit unit) throws InterruptedException { - return getOrCreateLock().tryLock(timeout, unit) ? this : null; - } - @Override public void close() { Objects.requireNonNull(lock, "close() called before acquire()").unlock(); diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 02623556498..e4c4b447cf6 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -3508,8 +3508,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3524,8 +3525,9 @@ class SentryClientTest { var terminated: Boolean? = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { terminated = isTerminating + return SentryId.EMPTY_ID } } ) @@ -3545,7 +3547,7 @@ class SentryClientTest { val replayId = SentryId() fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) {} + override fun captureReplay(isTerminating: Boolean?): SentryId = replayId } ) val sut = fixture.getSut() @@ -3603,8 +3605,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3625,8 +3628,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3665,8 +3669,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3685,8 +3690,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3705,8 +3711,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3721,8 +3728,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3742,7 +3750,7 @@ class SentryClientTest { var receivedHint: Hint? = null fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) {} + override fun captureReplay(isTerminating: Boolean?): SentryId = SentryId.EMPTY_ID } ) fixture.sentryOptions.sessionReplay.beforeErrorSampling = @@ -3765,8 +3773,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3928,7 +3937,7 @@ class SentryClientTest { val replayController = mock() val replayId = SentryId() val scope = createScope() - whenever(replayController.captureReplay(any())).thenAnswer { run { scope.replayId = replayId } } + whenever(replayController.captureReplay(any())).thenReturn(replayId) val sut = fixture.getSut { it.setReplayController(replayController) } // When there is no replay id in the feedback sut.captureFeedback(Feedback("message"), null, scope) @@ -3938,7 +3947,7 @@ class SentryClientTest { val sentFeedback = sentEvent!!.contexts.feedback assertNotNull(sentFeedback) - // And the replay id is set to the one from the scope (coming from the replay controller) + // And the replay id returned by the replay controller is set assertEquals(replayId, sentFeedback.replayId) } @@ -3952,7 +3961,7 @@ class SentryClientTest { val replayController = mock() val replayId = SentryId() val scope = createScope() - whenever(replayController.captureReplay(any())).thenAnswer { run { scope.replayId = replayId } } + whenever(replayController.captureReplay(any())).thenReturn(replayId) val sut = fixture.getSut { it.setReplayController(replayController) } // When there is replay id in the feedback val feedback = Feedback("message") diff --git a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt index b46cfbaea50..943a2c2bf70 100644 --- a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt +++ b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt @@ -1,6 +1,5 @@ package io.sentry.util -import com.google.common.truth.Truth.assertThat import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger @@ -42,55 +41,6 @@ class AutoClosableReentrantLockTest { assertFalse(lock.isLocked) } - @Test - fun `tryAcquire returns the lock itself as the token when free`() { - val lock = AutoClosableReentrantLock() - val token = lock.tryAcquire(1, TimeUnit.SECONDS) - assertThat(token).isSameInstanceAs(lock) - token!!.use { assertThat(lock.isLocked).isTrue() } - assertThat(lock.isLocked).isFalse() - } - - @Test - fun `tryAcquire does not allocate the underlying lock until first use`() { - val lock = AutoClosableReentrantLock() - assertThat(lock.isLockAllocated).isFalse() - lock.tryAcquire(1, TimeUnit.SECONDS)!!.use {} - assertThat(lock.isLockAllocated).isTrue() - } - - @Test - fun `tryAcquire returns null when another thread holds the lock past the timeout`() { - val lock = AutoClosableReentrantLock() - val acquired = CountDownLatch(1) - val release = CountDownLatch(1) - val holder = Thread { - lock.acquire().use { - acquired.countDown() - release.await() - } - } - holder.start() - try { - assertThat(acquired.await(10, TimeUnit.SECONDS)).isTrue() - assertThat(lock.tryAcquire(10, TimeUnit.MILLISECONDS)).isNull() - } finally { - release.countDown() - holder.join(TimeUnit.SECONDS.toMillis(10)) - } - assertThat(lock.isLocked).isFalse() - } - - @Test - fun `tryAcquire is reentrant from the same thread`() { - val lock = AutoClosableReentrantLock() - lock.acquire().use { - lock.tryAcquire(0, TimeUnit.MILLISECONDS)!!.use { assertThat(lock.isLocked).isTrue() } - assertThat(lock.isLocked).isTrue() - } - assertThat(lock.isLocked).isFalse() - } - @Test fun `mutually excludes concurrent threads`() { val lock = AutoClosableReentrantLock()