Skip to content

Commit 6702434

Browse files
committed
fix(compose): use deterministic ref-counting instead of weak cache for root spans
Wrapping the cached root span holder in a WeakReference fixed the prior leak but let GC collect it between recompositions even while a SentryTraced call under that scopes was still mounted, silently breaking the "sibling calls share one root span" guarantee. RootSpans now uses plain HashMaps with reference counting: each SentryTraced call retains its scopes entry via a DisposableEffect and releases it on dispose. An entry is only removed once the last SentryTraced call using that IScopes actually leaves composition, so cleanup no longer depends on GC timing.
1 parent 446749f commit 6702434

1 file changed

Lines changed: 31 additions & 14 deletions

File tree

sentry-compose/src/androidMain/kotlin/io/sentry/compose/SentryComposeTracing.kt

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package io.sentry.compose
33
import androidx.compose.foundation.layout.Box
44
import androidx.compose.foundation.layout.BoxScope
55
import androidx.compose.runtime.Composable
6+
import androidx.compose.runtime.DisposableEffect
67
import androidx.compose.runtime.Immutable
78
import androidx.compose.runtime.ProvidableCompositionLocal
89
import androidx.compose.runtime.remember
@@ -15,8 +16,6 @@ import io.sentry.ISpan
1516
import io.sentry.Sentry
1617
import io.sentry.SpanOptions
1718
import io.sentry.compose.SentryModifier.sentryTag
18-
import java.lang.ref.WeakReference
19-
import java.util.WeakHashMap
2019

2120
private const val OP_PARENT_COMPOSITION = "ui.compose.composition"
2221
private const val OP_COMPOSE = "ui.compose"
@@ -74,26 +73,40 @@ private fun createRenderingParentSpan(scopes: IScopes): ImmutableHolder<ISpan?>
7473
)
7574

7675
private class RootSpans {
77-
// Weakly keyed so scopes instances that are no longer referenced elsewhere (e.g. a short-lived
78-
// custom scopes provided via LocalSentryScopes for a finished screen/session) can be garbage
79-
// collected instead of being pinned here for the lifetime of the root Composition. The cached
80-
// holder is itself wrapped in a WeakReference: its ISpan strongly references the scopes that
81-
// created it (Span keeps its Scopes alive), so storing the holder directly as the map value
82-
// would let that strong value -> key path keep every scopes instance permanently reachable,
83-
// defeating the WeakHashMap keying entirely.
84-
val compositionSpans = WeakHashMap<IScopes, WeakReference<ImmutableHolder<ISpan?>>>()
85-
val renderingSpans = WeakHashMap<IScopes, WeakReference<ImmutableHolder<ISpan?>>>()
76+
// Entries are cleaned up deterministically via reference counting (see retain/release) rather
77+
// than relying on GC to reclaim a weakly-keyed map: a value here (ImmutableHolder -> Span)
78+
// strongly references the IScopes that created it (Span keeps its Scopes alive), so a
79+
// WeakHashMap keyed on IScopes would never actually evict its own key, and a value wrapped in a
80+
// WeakReference could be collected between recompositions even while a SentryTraced call under
81+
// that scopes is still in composition, silently breaking root span sharing.
82+
val compositionSpans = HashMap<IScopes, ImmutableHolder<ISpan?>>()
83+
val renderingSpans = HashMap<IScopes, ImmutableHolder<ISpan?>>()
84+
private val refCounts = HashMap<IScopes, Int>()
85+
86+
fun retain(scopes: IScopes) {
87+
refCounts[scopes] = (refCounts[scopes] ?: 0) + 1
88+
}
89+
90+
fun release(scopes: IScopes) {
91+
val remaining = (refCounts[scopes] ?: 1) - 1
92+
if (remaining <= 0) {
93+
refCounts.remove(scopes)
94+
compositionSpans.remove(scopes)
95+
renderingSpans.remove(scopes)
96+
} else {
97+
refCounts[scopes] = remaining
98+
}
99+
}
86100
}
87101

88102
private fun getOrCreateParentSpan(
89-
map: MutableMap<IScopes, WeakReference<ImmutableHolder<ISpan?>>>,
103+
map: MutableMap<IScopes, ImmutableHolder<ISpan?>>,
90104
scopes: IScopes,
91105
create: (IScopes) -> ImmutableHolder<ISpan?>,
92106
): ImmutableHolder<ISpan?> =
93107
// Only cache the holder once it actually contains a span; a null result (no transaction bound
94108
// to the scopes yet) is recomputed on the next call so a later transaction is still picked up.
95-
// A cleared reference (holder was collected) is also recomputed rather than reused.
96-
map[scopes]?.get() ?: create(scopes).also { if (it.item != null) map[scopes] = WeakReference(it) }
109+
map[scopes] ?: create(scopes).also { if (it.item != null) map[scopes] = it }
97110

98111
// Cached once per Composition and shared by every SentryTraced call within it, mirroring the
99112
// old eagerly-computed `compositionLocalOf { ... }` default (which Compose resolves once and
@@ -111,6 +124,10 @@ public fun SentryTraced(
111124
) {
112125
val scopes = LocalSentryScopes.current ?: Sentry.getCurrentScopes()
113126
val rootSpans = LocalRootSpans.current
127+
DisposableEffect(rootSpans, scopes) {
128+
rootSpans.retain(scopes)
129+
onDispose { rootSpans.release(scopes) }
130+
}
114131
val parentCompositionSpan =
115132
getOrCreateParentSpan(rootSpans.compositionSpans, scopes, ::createCompositionParentSpan)
116133
val parentRenderingSpan =

0 commit comments

Comments
 (0)