Skip to content

Commit 3f176a7

Browse files
committed
fix: guard executor shutdown in BaseCaptureStrategy.stop()
Each start/stop cycle leaked one SentryReplayPersister-* thread because stop() reset delegated properties (segmentTimestamp, currentReplayId) whose setters dispatch to persistingExecutor, initialising the lazy — but stop() never shut it down. Replace the lazy delegate with an explicit nullable holder so the executor is only created when actually needed and can be detected at stop() time. Call shutdownNow() (non-blocking) rather than the blocking shutdown() to avoid ANRs when stop() runs on the main thread. Fixes #5564
1 parent 2c01eff commit 3f176a7

2 files changed

Lines changed: 53 additions & 5 deletions

File tree

sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,20 @@ internal abstract class BaseCaptureStrategy(
5858
private const val MAX_TRACE_IDS = 100
5959
}
6060

61-
private val persistingExecutor: ScheduledExecutorService by lazy {
62-
val delegate =
63-
Executors.newSingleThreadScheduledExecutor(ReplayPersistingExecutorServiceThreadFactory())
64-
ReplayExecutorService(delegate, options)
65-
}
61+
// Explicit holder so we can detect whether the executor was ever initialised and shut it down
62+
// without initialising it as a side-effect (which is what a plain `lazy` delegate would do).
63+
private var persistingExecutorHolder: ScheduledExecutorService? = null
64+
private val persistingExecutor: ScheduledExecutorService
65+
get() {
66+
return persistingExecutorHolder
67+
?: run {
68+
val delegate =
69+
Executors.newSingleThreadScheduledExecutor(
70+
ReplayPersistingExecutorServiceThreadFactory()
71+
)
72+
ReplayExecutorService(delegate, options).also { persistingExecutorHolder = it }
73+
}
74+
}
6675
private val gestureConverter = ReplayGestureConverter(dateProvider)
6776

6877
protected val isTerminating = AtomicBoolean(false)
@@ -123,6 +132,13 @@ internal abstract class BaseCaptureStrategy(
123132
replayStartTimestamp.set(0)
124133
segmentTimestamp = null
125134
currentReplayId = SentryId.EMPTY_ID
135+
// Shut down the persisting executor only if it was actually initialised. We must not call the
136+
// blocking ReplayExecutorService.shutdown() here because stop() may run on the main thread,
137+
// and blocking there risks an ANR. shutdownNow() is non-blocking: it cancels queued tasks and
138+
// interrupts any running one, which is acceptable because the tasks are best-effort persistence
139+
// writes and the cache has already been closed above.
140+
persistingExecutorHolder?.shutdownNow()
141+
persistingExecutorHolder = null
126142
}
127143

128144
protected fun createSegmentInternal(

sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import io.sentry.IScopes
77
import io.sentry.Scope
88
import io.sentry.ScopeCallback
99
import io.sentry.SentryOptions
10+
import io.sentry.util.thread.IThreadChecker
1011
import io.sentry.SentryReplayEvent
1112
import io.sentry.SentryReplayEvent.ReplayType
1213
import io.sentry.SentryReplayOptions.SentryReplayQuality.HIGH
@@ -554,4 +555,35 @@ class SessionCaptureStrategyTest {
554555
any(),
555556
)
556557
}
558+
559+
@Test
560+
fun `stop shuts down persisting executor so no SentryReplayPersister threads leak across cycles`() {
561+
// Force isMainThread() to return true so that property-reset writes in stop() are dispatched
562+
// to the persistingExecutor, which is the code path that used to leak threads.
563+
fixture.options.threadChecker =
564+
mock<IThreadChecker> {
565+
on { isMainThread() }.thenReturn(true)
566+
on { isMainThread(any<Thread>()) }.thenReturn(true)
567+
on { isMainThread(anyLong()) }.thenReturn(false)
568+
on { currentThreadName }.thenReturn("main")
569+
on { currentThreadSystemId() }.thenReturn(0L)
570+
}
571+
572+
repeat(3) {
573+
val strategy = fixture.getSut()
574+
strategy.start(0, SentryId())
575+
strategy.onConfigurationChanged(fixture.recorderConfig)
576+
strategy.stop()
577+
}
578+
579+
// Allow time for thread termination after shutdownNow().
580+
Thread.sleep(200)
581+
582+
val leakedThreads =
583+
Thread.getAllStackTraces().keys.filter { it.name.startsWith("SentryReplayPersister-") }
584+
assertTrue(
585+
leakedThreads.isEmpty(),
586+
"Expected no SentryReplayPersister threads after stop(), found: ${leakedThreads.map { it.name }}",
587+
)
588+
}
557589
}

0 commit comments

Comments
 (0)