Skip to content

Commit d061260

Browse files
tsushanthromtsnclaude
authored
fix: guard executor shutdown in BaseCaptureStrategy.stop() (#5627)
* 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 * style: apply spotless formatting * refactor(replay): move persistingExecutor ownership to ReplayIntegration Move persistingExecutor out of BaseCaptureStrategy and into ReplayIntegration, passing it as a constructor argument to CaptureStrategy subclasses. Shut it down in ReplayIntegration.close() alongside replayExecutor so executor lifecycle is managed in one place. * Fix leak in ReplayIntegration due to persisting executor not being shut down Add the persistingExecutor argument to SessionCaptureStrategy and BufferCaptureStrategy constructor calls in tests, and add changelog entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove stray merge conflict marker from CHANGELOG.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove no-op leak test from SessionCaptureStrategyTest The test used a mocked executor that never spawned threads, so the thread-count assertion was always true regardless of the fix. The executor lifecycle is now owned by ReplayIntegration, not SessionCaptureStrategy, so the test belonged at the wrong layer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add executor leak regression test to ReplayIntegrationTest Uses real ScheduledThreadPoolExecutor threads so the test actually fails if the shutdown in close() is removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Use shutdownNow() for replay executors in close() to avoid ANR shutdown() calls awaitTermination() which blocks up to shutdownTimeoutMillis. Since close() can run on the main thread (via Sentry.close() from hybrid SDKs), this risks an ANR. shutdownNow() is non-blocking and sufficient at teardown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 58b65f0 commit d061260

9 files changed

Lines changed: 101 additions & 23 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
- Don't start a redundant UI interaction transaction when a transaction is already bound to the Scope ([#5491](https://github.com/getsentry/sentry-java/issues/5491))
1212
- Previously, `SentryGestureListener` always started a UI transaction and only afterwards skipped binding it to the Scope when a manually-bound transaction already existed, leaving the new transaction to be dropped as an idle transaction without children.
1313
- Fix potential NPE within `Scope.endSession()` ([#5657](https://github.com/getsentry/sentry-java/pull/5657))
14+
- Fix memory leak in `ReplayIntegration` due to persisting executor not being shut down ([#5627](https://github.com/getsentry/sentry-java/pull/5627))
1415

1516
### Performance
1617

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,17 @@ public class ReplayIntegration(
107107
private var gestureRecorder: GestureRecorder? = null
108108
private val random by lazy { Random() }
109109
internal val rootViewsSpy by lazy { RootViewsSpy.install() }
110-
private val replayExecutor by lazy {
110+
internal val lazyReplayExecutor = lazy {
111111
val delegate = Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory())
112112
ReplayExecutorService(delegate, options)
113113
}
114+
internal val replayExecutor by lazyReplayExecutor
115+
internal val lazyPersistingExecutor = lazy {
116+
val delegate =
117+
Executors.newSingleThreadScheduledExecutor(ReplayPersistingExecutorServiceThreadFactory())
118+
ReplayExecutorService(delegate, options)
119+
}
120+
internal val persistingExecutor by lazyPersistingExecutor
114121

115122
internal val isEnabled = AtomicBoolean(false)
116123
internal val isManualPause = AtomicBoolean(false)
@@ -192,6 +199,7 @@ public class ReplayIntegration(
192199
scopes,
193200
dateProvider,
194201
replayExecutor,
202+
persistingExecutor,
195203
replayCacheProvider,
196204
)
197205
} else {
@@ -201,6 +209,7 @@ public class ReplayIntegration(
201209
dateProvider,
202210
random,
203211
replayExecutor,
212+
persistingExecutor,
204213
replayCacheProvider,
205214
)
206215
}
@@ -373,7 +382,20 @@ public class ReplayIntegration(
373382
recorder?.close()
374383
recorder = null
375384
rootViewsSpy.close()
376-
replayExecutor.shutdown()
385+
if (lazyReplayExecutor.isInitialized()) {
386+
if (options.threadChecker.isMainThread) {
387+
replayExecutor.gracefulShutdown()
388+
} else {
389+
replayExecutor.shutdown()
390+
}
391+
}
392+
if (lazyPersistingExecutor.isInitialized()) {
393+
if (options.threadChecker.isMainThread) {
394+
persistingExecutor.gracefulShutdown()
395+
} else {
396+
persistingExecutor.shutdown()
397+
}
398+
}
377399
lifecycle.currentState = CLOSED
378400
}
379401
}
@@ -554,4 +576,14 @@ public class ReplayIntegration(
554576
return ret
555577
}
556578
}
579+
580+
private class ReplayPersistingExecutorServiceThreadFactory : ThreadFactory {
581+
private var cnt = 0
582+
583+
override fun newThread(r: Runnable): Thread {
584+
val ret = Thread(r, "SentryReplayPersister-" + cnt++)
585+
ret.setDaemon(true)
586+
return ret
587+
}
588+
}
557589
}

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

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import io.sentry.android.replay.ScreenshotRecorderConfig
2525
import io.sentry.android.replay.capture.CaptureStrategy.Companion.createSegment
2626
import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment
2727
import io.sentry.android.replay.gestures.ReplayGestureConverter
28-
import io.sentry.android.replay.util.ReplayExecutorService
2928
import io.sentry.android.replay.util.ReplayRunnable
3029
import io.sentry.protocol.SentryId
3130
import io.sentry.rrweb.RRWebEvent
@@ -34,9 +33,7 @@ import java.io.File
3433
import java.util.Date
3534
import java.util.Deque
3635
import java.util.concurrent.ConcurrentLinkedDeque
37-
import java.util.concurrent.Executors
3836
import java.util.concurrent.ScheduledExecutorService
39-
import java.util.concurrent.ThreadFactory
4037
import java.util.concurrent.atomic.AtomicBoolean
4138
import java.util.concurrent.atomic.AtomicLong
4239
import java.util.concurrent.atomic.AtomicReference
@@ -50,6 +47,7 @@ internal abstract class BaseCaptureStrategy(
5047
private val scopes: IScopes?,
5148
private val dateProvider: ICurrentDateProvider,
5249
protected val replayExecutor: ScheduledExecutorService,
50+
protected val persistingExecutor: ScheduledExecutorService,
5351
private val replayCacheProvider: ((replayId: SentryId) -> ReplayCache)? = null,
5452
) : CaptureStrategy {
5553
internal companion object {
@@ -58,11 +56,6 @@ internal abstract class BaseCaptureStrategy(
5856
private const val MAX_TRACE_IDS = 100
5957
}
6058

61-
private val persistingExecutor: ScheduledExecutorService by lazy {
62-
val delegate =
63-
Executors.newSingleThreadScheduledExecutor(ReplayPersistingExecutorServiceThreadFactory())
64-
ReplayExecutorService(delegate, options)
65-
}
6659
private val gestureConverter = ReplayGestureConverter(dateProvider)
6760

6861
protected val isTerminating = AtomicBoolean(false)
@@ -192,16 +185,6 @@ internal abstract class BaseCaptureStrategy(
192185
}
193186
}
194187

195-
private class ReplayPersistingExecutorServiceThreadFactory : ThreadFactory {
196-
private var cnt = 0
197-
198-
override fun newThread(r: Runnable): Thread {
199-
val ret = Thread(r, "SentryReplayPersister-" + cnt++)
200-
ret.setDaemon(true)
201-
return ret
202-
}
203-
}
204-
205188
private inline fun <T> persistableAtomicNullable(
206189
initialValue: T? = null,
207190
propertyName: String,

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,15 @@ internal class BufferCaptureStrategy(
3333
private val dateProvider: ICurrentDateProvider,
3434
private val random: Random,
3535
executor: ScheduledExecutorService,
36+
persistingExecutor: ScheduledExecutorService,
3637
replayCacheProvider: ((replayId: SentryId) -> ReplayCache)? = null,
3738
) :
3839
BaseCaptureStrategy(
3940
options,
4041
scopes,
4142
dateProvider,
4243
executor,
44+
persistingExecutor,
4345
replayCacheProvider = replayCacheProvider,
4446
) {
4547
// TODO: capture envelopes for buffered segments instead, but don't send them until buffer is
@@ -150,8 +152,10 @@ internal class BufferCaptureStrategy(
150152
)
151153
return this
152154
}
153-
// we hand over replayExecutor to the new strategy to preserve order of execution
154-
val captureStrategy = SessionCaptureStrategy(options, scopes, dateProvider, replayExecutor)
155+
// we hand over replayExecutor and persistingExecutor to the new strategy to preserve order of
156+
// execution
157+
val captureStrategy =
158+
SessionCaptureStrategy(options, scopes, dateProvider, replayExecutor, persistingExecutor)
155159
captureStrategy.recorderConfig = recorderConfig
156160
captureStrategy.start(
157161
segmentId = currentSegment,

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,17 @@ internal class SessionCaptureStrategy(
2121
private val scopes: IScopes?,
2222
private val dateProvider: ICurrentDateProvider,
2323
executor: ScheduledExecutorService,
24+
persistingExecutor: ScheduledExecutorService,
2425
replayCacheProvider: ((replayId: SentryId) -> ReplayCache)? = null,
25-
) : BaseCaptureStrategy(options, scopes, dateProvider, executor, replayCacheProvider) {
26+
) :
27+
BaseCaptureStrategy(
28+
options,
29+
scopes,
30+
dateProvider,
31+
executor,
32+
persistingExecutor,
33+
replayCacheProvider,
34+
) {
2635
internal companion object {
2736
private const val TAG = "SessionCaptureStrategy"
2837
}

sentry-android-replay/src/main/java/io/sentry/android/replay/util/ReplayExecutorService.kt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ internal class ReplayExecutorService(
5757
}
5858
}
5959
}
60+
61+
fun gracefulShutdown() {
62+
synchronized(this) {
63+
if (!isShutdown) {
64+
delegate.shutdown()
65+
}
66+
}
67+
}
6068
}
6169

6270
internal class ReplayRunnable(val taskName: String, delegate: Runnable) : Runnable by delegate

sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,12 @@ class ReplayIntegrationTest {
754754
null
755755
}
756756
},
757+
mock {
758+
whenever(mock.submit(any<Runnable>())).doAnswer {
759+
(it.arguments[0] as Runnable).run()
760+
null
761+
}
762+
},
757763
) { _ ->
758764
fixture.replayCache
759765
}
@@ -1104,6 +1110,20 @@ class ReplayIntegrationTest {
11041110
assertEquals(traceId, traceIdRegistered)
11051111
}
11061112

1113+
@Test
1114+
fun `close shuts down replay executors`() {
1115+
fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath
1116+
1117+
val replay = fixture.getSut(context)
1118+
replay.register(fixture.scopes, fixture.options)
1119+
replay.start()
1120+
replay.stop()
1121+
replay.close()
1122+
1123+
assertTrue(replay.replayExecutor.isShutdown)
1124+
assertTrue(replay.persistingExecutor.isShutdown)
1125+
}
1126+
11071127
private fun getSessionCaptureStrategy(options: SentryOptions): SessionCaptureStrategy =
11081128
SessionCaptureStrategy(
11091129
options,
@@ -1116,5 +1136,12 @@ class ReplayIntegrationTest {
11161136
null
11171137
}
11181138
},
1139+
persistingExecutor =
1140+
mock {
1141+
whenever(mock.submit(any<Runnable>())).doAnswer {
1142+
(it.arguments[0] as Runnable).run()
1143+
null
1144+
}
1145+
},
11191146
)
11201147
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,12 @@ class BufferCaptureStrategyTest {
111111
null
112112
}
113113
},
114+
mock {
115+
whenever(it.submit(any<Runnable>())).doAnswer { invocation ->
116+
(invocation.arguments[0] as Runnable).run()
117+
null
118+
}
119+
},
114120
) { _ ->
115121
replayCache
116122
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,14 @@ class SessionCaptureStrategyTest {
122122
.whenever(it)
123123
.submit(any<Runnable>())
124124
},
125+
mock {
126+
doAnswer { invocation ->
127+
(invocation.arguments[0] as Runnable).run()
128+
null
129+
}
130+
.whenever(it)
131+
.submit(any<Runnable>())
132+
},
125133
) { _ ->
126134
replayCache
127135
}

0 commit comments

Comments
 (0)