diff --git a/.cursor/rules/scopes.mdc b/.cursor/rules/scopes.mdc index e054755d4f5..6da9baeec39 100644 --- a/.cursor/rules/scopes.mdc +++ b/.cursor/rules/scopes.mdc @@ -90,15 +90,20 @@ For JVM Backend applications (servers) we discourage enabling `globalHubMode` si ### Enabled -If `globalHubMode` is enabled, the SDK avoids forking scopes. +If `globalHubMode` is enabled, the SDK avoids forking scopes _implicitly_. This means, retrieving current scopes on a thread where specific scopes do not exist yet for the thread, the root scopes are not forked but returned directly. -The SDK also doesn't fork scopes when `Sentry.pushScope`, `Sentry.pushIsolation`, `Sentry.withScope` or `Sentry.withIsolationScope` are executed. +`Sentry.pushScope`, `Sentry.pushIsolationScope` and `Sentry.popScope` are no-ops. +They are unbalanced API: the caller may never restore the previous scopes, or restore them on a different thread, which would corrupt the globally shared scopes. + +`Sentry.withScope` and `Sentry.withIsolationScope` do fork, also when `globalHubMode` is enabled. +They are balanced by construction and always restore the previous scopes before returning, so they cannot corrupt the shared scopes. +This matches the cross SDK spec ("fork the current scope, invoke callback, discard the fork when done") and what the SDK did before major version 8. The suppression of forking via `globalHubMode` only applies when using `Sentry` static API or `ScopesAdapter`. In case the `Scopes` instance is accessed directly, forking will happen as if `globalHubMode` is disabled. -However, while it's possible to use `Sentry.setCurrentScopes` it does not have any effect due to `Sentry.getCurrentScopes` directly returning `rootScopes` if `globalHubMode` is enabled. -This means the forked scopes have to be managed manually, e.g. by keeping a reference and accessing Sentry API via the reference instead of using static API. +`Sentry.setCurrentScopes`, and thus `Scopes.makeCurrent`, does have an effect when `globalHubMode` is enabled, but only for scopes that descend from the current `rootScopes`. +`Sentry.getCurrentScopes` ignores all other scopes, because scopes left over from a closed or re-initialized SDK would otherwise be read back from a thread local that the SDK cannot clean up. `ScopesAdapter` makes use of the static `Sentry` API internally. It allows us to access the correct scopes for the current context without passing it along explicitly. It also makes testing easier. diff --git a/CHANGELOG.md b/CHANGELOG.md index c9fd18124a7..9579bd744f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ ### Fixes - Keep dropped tombstone and ANR events dropped, instead of reporting the same app exit again at every app start ([#6002](https://github.com/getsentry/sentry-java/pull/6002)) +- Apply `Sentry.withScope` and `Sentry.withIsolationScope` data to events captured inside the callback when `globalHubMode` is enabled ([#6004](https://github.com/getsentry/sentry-java/pull/6004)) + - `globalHubMode` is enabled by default on Android, where tags, extras, contexts and level set inside the callback were silently dropped + - Scopes that are explicitly made current, e.g. via `Sentry.setCurrentScopes` or the `SentryContext` coroutine integration, are now also honoured when `globalHubMode` is enabled + - `Sentry.pushScope`, `Sentry.pushIsolationScope` and `Sentry.popScope` remain no-ops when `globalHubMode` is enabled ## 8.54.0 diff --git a/sentry-kotlin-extensions/src/test/java/io/sentry/kotlin/SentryContextTest.kt b/sentry-kotlin-extensions/src/test/java/io/sentry/kotlin/SentryContextTest.kt index cec0baa4ce7..55fa4f58d6a 100644 --- a/sentry-kotlin-extensions/src/test/java/io/sentry/kotlin/SentryContextTest.kt +++ b/sentry-kotlin-extensions/src/test/java/io/sentry/kotlin/SentryContextTest.kt @@ -11,18 +11,20 @@ import kotlin.test.assertNotEquals import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlinx.coroutines.CoroutineName +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.joinAll import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking class SentryContextTest { - // TODO [HSM] In global hub mode SentryContext behaves differently - // because Sentry.getCurrentScopes always returns rootScopes - // What's the desired behaviour? + // SentryContext makes its scopes current, which is honoured in global hub mode as well, so it + // isolates the same way there. Only implicit forking stays suppressed, see the last two tests. + + private val dsn = "https://key@sentry.io/123" @BeforeTest fun init() { - initForTest("https://key@sentry.io/123") + initForTest(dsn) } @AfterTest @@ -264,6 +266,46 @@ class SentryContextTest { assertEquals(initialContextElement, mergedContextElement) } + @Test + fun `current scope is isolated between coroutines in global hub mode`() = runBlocking { + initForTest({ it.dsn = dsn }, true) + Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("parent", "parentValue") } + + val c1 = + launch(SentryContext()) { + Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("c1", "c1value") } + assertEquals("c1value", getTag("c1", ScopeType.CURRENT)) + assertEquals("parentValue", getTag("parent", ScopeType.CURRENT)) + assertNull(getTag("c2", ScopeType.CURRENT)) + } + val c2 = + launch(SentryContext()) { + Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("c2", "c2value") } + assertEquals("c2value", getTag("c2", ScopeType.CURRENT)) + assertEquals("parentValue", getTag("parent", ScopeType.CURRENT)) + assertNull(getTag("c1", ScopeType.CURRENT)) + } + listOf(c1, c2).joinAll() + + assertNotNull(getTag("parent", ScopeType.CURRENT)) + assertNull(getTag("c1", ScopeType.CURRENT)) + assertNull(getTag("c2", ScopeType.CURRENT)) + } + + @Test + fun `coroutines without SentryContext share the root scopes in global hub mode`() = runBlocking { + initForTest({ it.dsn = dsn }, true) + Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("parent", "parentValue") } + + launch(Dispatchers.Default) { + assertEquals("parentValue", getTag("parent", ScopeType.CURRENT)) + Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("child", "childValue") } + } + .join() + + assertEquals("childValue", getTag("child", ScopeType.CURRENT)) + } + private fun getTag(tag: String, scopeType: ScopeType = ScopeType.ISOLATION): String? { var value: String? = null Sentry.configureScope(scopeType) { value = it.tags[tag] } diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/webflux/SentryWebfluxIntegrationTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/webflux/SentryWebfluxIntegrationTest.kt index a91023b4ba0..56a7c07df41 100644 --- a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/webflux/SentryWebfluxIntegrationTest.kt +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/webflux/SentryWebfluxIntegrationTest.kt @@ -127,11 +127,14 @@ class SentryWebfluxIntegrationTest { .expectStatus() .isOk - verify(transport) - .send( - checkTransaction { event -> assertEquals("GET /hello", event.transaction) }, - anyOrNull(), - ) + // the transaction is finished asynchronously, after the response has been sent + await.untilAsserted { + verify(transport) + .send( + checkTransaction { event -> assertEquals("GET /hello", event.transaction) }, + anyOrNull(), + ) + } } } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt index b442594f150..f1ace818f6d 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt @@ -127,11 +127,14 @@ class SentryWebfluxIntegrationTest { .expectStatus() .isOk - verify(transport) - .send( - checkTransaction { event -> assertEquals("GET /hello", event.transaction) }, - anyOrNull(), - ) + // the transaction is finished asynchronously, after the response has been sent + await.untilAsserted { + verify(transport) + .send( + checkTransaction { event -> assertEquals("GET /hello", event.transaction) }, + anyOrNull(), + ) + } } } diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 266aa39e793..03288ae00ad 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -114,15 +114,24 @@ private Sentry() {} @ApiStatus.Internal @SuppressWarnings("deprecation") public static @NotNull IScopes getCurrentScopes(final boolean ensureForked) { + // read the volatile rootScopes once, so a concurrent Sentry.init cannot make the check below + // disagree with what we return + final @NotNull IScopes root = rootScopes; + @Nullable IScopes scopes = getScopesStorage().get(); if (globalHubMode) { - return rootScopes; + // in global hub mode we never fork implicitly, but scopes that have explicitly been made + // current (e.g. by withScope) must still be honoured. Anything that did not originate from + // the present rootScopes is stale (SDK closed or re-initialized) and gets ignored. + if (scopes != null && !scopes.isNoOp() && root.isAncestorOf(scopes)) { + return scopes; + } + return root; } - @Nullable IScopes scopes = getScopesStorage().get(); if (scopes == null || scopes.isNoOp()) { if (!ensureForked) { return NoOpScopes.getInstance(); } else { - scopes = rootScopes.forkedScopes("getCurrentScopes"); + scopes = root.forkedScopes("getCurrentScopes"); getScopesStorage().set(scopes); } } @@ -1060,7 +1069,13 @@ public static void removeExtra(final @Nullable String key) { return getCurrentScopes().getLastEventId(); } - /** Pushes a new scope while inheriting the current scope's data. */ + /** + * Pushes a new scope while inheriting the current scope's data. + * + *
This is a no-op in global hub mode, as the caller may never pop the scope again, or pop it + * on a different thread, which would corrupt the globally shared scopes. Use {@link + * Sentry#withScope(ScopeCallback)} if you need forking that also works in global hub mode. + */ public static @NotNull ISentryLifecycleToken pushScope() { // pushScope is no-op in global hub mode if (!globalHubMode) { @@ -1069,7 +1084,12 @@ public static void removeExtra(final @Nullable String key) { return NoOpScopesLifecycleToken.getInstance(); } - /** Pushes a new isolation and current scope while inheriting the current scope's data. */ + /** + * Pushes a new isolation and current scope while inheriting the current scope's data. + * + *
This is a no-op in global hub mode, for the same reason as {@link Sentry#pushScope()}. Use + * {@link Sentry#withIsolationScope(ScopeCallback)} instead. + */ public static @NotNull ISentryLifecycleToken pushIsolationScope() { // pushScope is no-op in global hub mode if (!globalHubMode) { @@ -1093,7 +1113,10 @@ public static void popScope() { } /** - * Runs the callback with a new current scope which gets dropped at the end + * Runs the callback with a new current scope which gets dropped at the end. + * + *
Unlike {@link Sentry#pushScope()} this also forks in global hub mode, since the previous + * scopes are always restored once the callback returns. * * @param callback the callback */ @@ -1105,6 +1128,9 @@ public static void withScope(final @NotNull ScopeCallback callback) { * Runs the callback with a new isolation scope which gets dropped at the end. Current scope is * also forked. * + *
Unlike {@link Sentry#pushIsolationScope()} this also forks in global hub mode, since the
+ * previous scopes are always restored once the callback returns.
+ *
* @param callback the callback
*/
public static void withIsolationScope(final @NotNull ScopeCallback callback) {
diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt
index 00f5f89db39..3d91c0f8ead 100644
--- a/sentry/src/test/java/io/sentry/SentryTest.kt
+++ b/sentry/src/test/java/io/sentry/SentryTest.kt
@@ -1,5 +1,6 @@
package io.sentry
+import com.google.common.truth.Truth.assertThat
import io.sentry.SentryFeedbackOptions.IFormHandler
import io.sentry.SentryOptions.ProfilesSamplerCallback
import io.sentry.SentryOptions.TracesSamplerCallback
@@ -1434,6 +1435,93 @@ class SentryTest {
assertNotSame(s1, s2)
}
+ private fun initCapturingEvents(globalHubMode: Boolean): MutableList