From 3c93c167c3c2af9e25de6449b710c0ab0c430375 Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Wed, 28 May 2025 15:53:28 +0200 Subject: [PATCH 1/6] allow multiple UncaughtExceptionHandlerIntegrations to be active. one per GlobalScope --- .../UncaughtExceptionHandlerIntegration.java | 80 +++++++---- ...UncaughtExceptionHandlerIntegrationTest.kt | 125 +++++++++++++++++- 2 files changed, 181 insertions(+), 24 deletions(-) diff --git a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java index 1cf4c151b0c..dd3bc8e86b5 100644 --- a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java +++ b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java @@ -10,6 +10,7 @@ import io.sentry.hints.TransactionEnd; import io.sentry.protocol.Mechanism; import io.sentry.protocol.SentryId; +import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.HintUtils; import io.sentry.util.Objects; import java.io.Closeable; @@ -28,6 +29,8 @@ public final class UncaughtExceptionHandlerIntegration /** Reference to the pre-existing uncaught exception handler. */ private @Nullable Thread.UncaughtExceptionHandler defaultExceptionHandler; + private static final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + private @Nullable IScopes scopes; private @Nullable SentryOptions options; @@ -65,27 +68,33 @@ public final void register(final @NotNull IScopes scopes, final @NotNull SentryO this.options.isEnableUncaughtExceptionHandler()); if (this.options.isEnableUncaughtExceptionHandler()) { - final Thread.UncaughtExceptionHandler currentHandler = - threadAdapter.getDefaultUncaughtExceptionHandler(); - if (currentHandler != null) { - this.options - .getLogger() - .log( - SentryLevel.DEBUG, - "default UncaughtExceptionHandler class='" - + currentHandler.getClass().getName() - + "'"); - - if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { - final UncaughtExceptionHandlerIntegration currentHandlerIntegration = - (UncaughtExceptionHandlerIntegration) currentHandler; - defaultExceptionHandler = currentHandlerIntegration.defaultExceptionHandler; - } else { - defaultExceptionHandler = currentHandler; + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + final Thread.UncaughtExceptionHandler currentHandler = + threadAdapter.getDefaultUncaughtExceptionHandler(); + if (currentHandler != null) { + this.options + .getLogger() + .log( + SentryLevel.DEBUG, + "default UncaughtExceptionHandler class='" + + currentHandler.getClass().getName() + + "'"); + if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { + final UncaughtExceptionHandlerIntegration currentHandlerIntegration = + (UncaughtExceptionHandlerIntegration) currentHandler; + if (currentHandlerIntegration.scopes != null + && scopes.getGlobalScope() == currentHandlerIntegration.scopes.getGlobalScope()) { + defaultExceptionHandler = currentHandlerIntegration.defaultExceptionHandler; + } else { + defaultExceptionHandler = currentHandler; + } + } else { + defaultExceptionHandler = currentHandler; + } } - } - threadAdapter.setDefaultUncaughtExceptionHandler(this); + threadAdapter.setDefaultUncaughtExceptionHandler(this); + } this.options .getLogger() @@ -159,11 +168,36 @@ static Throwable getUnhandledThrowable( @Override public void close() { - if (this == threadAdapter.getDefaultUncaughtExceptionHandler()) { - threadAdapter.setDefaultUncaughtExceptionHandler(defaultExceptionHandler); + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + if (this == threadAdapter.getDefaultUncaughtExceptionHandler()) { + threadAdapter.setDefaultUncaughtExceptionHandler(defaultExceptionHandler); + + if (options != null) { + options + .getLogger() + .log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed."); + } + } else { + removeFromHandlerTree(threadAdapter.getDefaultUncaughtExceptionHandler()); + } + } + } - if (options != null) { - options.getLogger().log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed."); + private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler currentHandler) { + if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { + final UncaughtExceptionHandlerIntegration currentHandlerIntegration = + (UncaughtExceptionHandlerIntegration) currentHandler; + if (this == currentHandlerIntegration.defaultExceptionHandler) { + currentHandlerIntegration.defaultExceptionHandler = defaultExceptionHandler; + + if (options != null) { + options + .getLogger() + .log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed."); + } + + } else { + removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler); } } } diff --git a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt index 409d3b971b1..2bff144824b 100644 --- a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt @@ -19,6 +19,8 @@ import org.mockito.kotlin.whenever import java.io.ByteArrayOutputStream import java.io.PrintStream import java.nio.file.Files +import java.util.concurrent.CompletableFuture +import java.util.concurrent.Executors import kotlin.concurrent.thread import kotlin.test.Test import kotlin.test.assertEquals @@ -313,7 +315,7 @@ class UncaughtExceptionHandlerIntegrationTest { val integration2 = UncaughtExceptionHandlerIntegration(handler) integration2.register(fixture.scopes, fixture.options) - assertEquals(currentDefaultHandler, integration2) + assertEquals(integration2, currentDefaultHandler) integration2.close() assertEquals(null, currentDefaultHandler) @@ -344,4 +346,125 @@ class UncaughtExceptionHandlerIntegrationTest { assertEquals(initialUncaughtExceptionHandler, currentDefaultHandler) } + + @Test + fun `multiple registrations with different global scopes allowed`() { + val scopes2 = mock() + val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> } + + var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler + + val handler = mock() + whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler } + + whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull())).then { + currentDefaultHandler = it.getArgument(0) + null + } + + whenever(scopes2.globalScope).thenReturn(mock()) + + val integration1 = UncaughtExceptionHandlerIntegration(handler) + integration1.register(fixture.scopes, fixture.options) + + val integration2 = UncaughtExceptionHandlerIntegration(handler) + integration2.register(scopes2, fixture.options) + + assertEquals(currentDefaultHandler, integration2) + integration2.close() + + assertEquals(integration1, currentDefaultHandler) + integration1.close() + + assertEquals(initialUncaughtExceptionHandler, currentDefaultHandler) + } + + @Test + fun `multiple registrations with different global scopes allowed, closed out of order`() { + fixture.getSut() + val scopes2 = mock() + val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> } + + var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler + + val handler = mock() + whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler } + + whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull())).then { + currentDefaultHandler = it.getArgument(0) + null + } + + whenever(scopes2.globalScope).thenReturn(mock()) + + val integration1 = UncaughtExceptionHandlerIntegration(handler) + integration1.register(fixture.scopes, fixture.options) + + val integration2 = UncaughtExceptionHandlerIntegration(handler) + integration2.register(scopes2, fixture.options) + + assertEquals(currentDefaultHandler, integration2) + integration1.close() + + assertEquals(integration2, currentDefaultHandler) + integration2.close() + + assertEquals(initialUncaughtExceptionHandler, currentDefaultHandler) + } + + @Test + fun `multiple registrations async, closed async, one remains`() { + val executor = Executors.newFixedThreadPool(4) + fixture.getSut() + val scopes2 = mock() + val scopes3 = mock() + val scopes4 = mock() + val scopes5 = mock() + + val scopesList = listOf(fixture.scopes, scopes2, scopes3, scopes4, scopes5) + + val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> } + + var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler + + val handler = mock() + whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler } + + whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull())).then { + currentDefaultHandler = it.getArgument(0) + null + } + + whenever(scopes2.globalScope).thenReturn(mock()) + whenever(scopes3.globalScope).thenReturn(mock()) + whenever(scopes4.globalScope).thenReturn(mock()) + whenever(scopes5.globalScope).thenReturn(mock()) + + val integrations = scopesList.map { scope -> + CompletableFuture.supplyAsync( + { + UncaughtExceptionHandlerIntegration(handler).apply { + register(scope, fixture.options) + } + }, + executor + ) + } + + CompletableFuture.allOf(*integrations.toTypedArray()).get() + + val futures = integrations.minus(integrations[2]).reversed().map { integration -> + CompletableFuture.supplyAsync( + { + integration.get().close() + println(Thread.currentThread().name) + }, + executor + ) + } + + CompletableFuture.allOf(*futures.toTypedArray()).get() + + assertEquals(integrations[2].get(), currentDefaultHandler) + } } From bcf3f848750c1a4c63d24a397e4bfd29b5d50018 Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Wed, 28 May 2025 16:12:39 +0200 Subject: [PATCH 2/6] Add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80e9cef14d4..72bcff1f293 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Allow multiple UncaughtExceptionHandlerIntegrations to be active at the same time ([#4462](https://github.com/getsentry/sentry-java/pull/4462)) + ## 8.13.2 ### Fixes From 0f1f0f44a77267cb27db4121bc9dfe124aeafb3a Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Tue, 24 Jun 2025 13:21:55 +0200 Subject: [PATCH 3/6] add comments to close and removeFromHandlerTree methods --- .../UncaughtExceptionHandlerIntegration.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java index dd3bc8e86b5..a008e57da05 100644 --- a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java +++ b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java @@ -166,6 +166,12 @@ static Throwable getUnhandledThrowable( return new ExceptionMechanismException(mechanism, thrown, thread); } + /** + * Remove this UncaughtExceptionHandlerIntegration from the exception handler chain. + * + *

If this integration is currently the default handler, restore the initial handler, if this + * integration is not the current default call removeFromHandlerTree + */ @Override public void close() { try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { @@ -183,6 +189,21 @@ public void close() { } } + /** + * Recursively traverses the chain of UncaughtExceptionHandlerIntegrations to find and remove this + * specific integration instance. + * + *

Checks if this instance is the defaultExceptionHandler of the current handler, if so replace + * with its own defaultExceptionHandler, thus removing it from the chain. + * + *

If not, recursively calls itself on the next handler in + * the chain. + * + *

Recursion stops if the current handler is not an instance of + * UncaughtExceptionHandlerIntegration or the handler was found and removed. + * + * @param currentHandler The current handler in the chain to examine + */ private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler currentHandler) { if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { final UncaughtExceptionHandlerIntegration currentHandlerIntegration = From f9a5cab17638a522e2d2a298c2a7eb1175c9a2fa Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Mon, 30 Jun 2025 09:29:45 +0200 Subject: [PATCH 4/6] add initial cycle detection --- .../UncaughtExceptionHandlerIntegration.java | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java index a008e57da05..23c881c44f0 100644 --- a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java +++ b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java @@ -14,6 +14,8 @@ import io.sentry.util.HintUtils; import io.sentry.util.Objects; import java.io.Closeable; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -189,6 +191,14 @@ public void close() { } } + /** + * Intermediary method before calling the actual recursive method. Used to initialize HashSet to + * keep track of visited handlers to avoid infinite recursion in case of cycles in the chain. + */ + private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler currentHandler) { + removeFromHandlerTree(currentHandler, new HashSet<>()); + } + /** * Recursively traverses the chain of UncaughtExceptionHandlerIntegrations to find and remove this * specific integration instance. @@ -200,11 +210,32 @@ public void close() { * the chain. * *

Recursion stops if the current handler is not an instance of - * UncaughtExceptionHandlerIntegration or the handler was found and removed. + * UncaughtExceptionHandlerIntegration, the handler was found and removed or a cycle was detected. * * @param currentHandler The current handler in the chain to examine + * @param visited Set of already visited handlers to detect cycles */ - private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler currentHandler) { + private void removeFromHandlerTree( + @Nullable Thread.UncaughtExceptionHandler currentHandler, + @NotNull Set visited) { + if (currentHandler != null) { + if (!visited.add(currentHandler)) { + if (options != null) { + options + .getLogger() + .log( + SentryLevel.WARNING, + "Cycle detected in UncaughtExceptionHandler chain while removing handler."); + } + return; + } + } else { + if (options != null) { + options.getLogger().log(SentryLevel.DEBUG, "Found no current handler to remove."); + } + return; + } + if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { final UncaughtExceptionHandlerIntegration currentHandlerIntegration = (UncaughtExceptionHandlerIntegration) currentHandler; @@ -218,7 +249,7 @@ private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler cur } } else { - removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler); + removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler, visited); } } } From db374ab24cea6ea5957975f2a39f3912afa9ecd0 Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Mon, 30 Jun 2025 09:58:52 +0200 Subject: [PATCH 5/6] make recursive method more readable --- .../UncaughtExceptionHandlerIntegration.java | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java index 23c881c44f0..8ca02cb8550 100644 --- a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java +++ b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java @@ -206,8 +206,7 @@ private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler cur *

Checks if this instance is the defaultExceptionHandler of the current handler, if so replace * with its own defaultExceptionHandler, thus removing it from the chain. * - *

If not, recursively calls itself on the next handler in - * the chain. + *

If not, recursively calls itself on the next handler in the chain. * *

Recursion stops if the current handler is not an instance of * UncaughtExceptionHandlerIntegration, the handler was found and removed or a cycle was detected. @@ -218,39 +217,39 @@ private void removeFromHandlerTree(@Nullable Thread.UncaughtExceptionHandler cur private void removeFromHandlerTree( @Nullable Thread.UncaughtExceptionHandler currentHandler, @NotNull Set visited) { - if (currentHandler != null) { - if (!visited.add(currentHandler)) { - if (options != null) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Cycle detected in UncaughtExceptionHandler chain while removing handler."); - } - return; + + if (currentHandler == null) { + if (options != null) { + options.getLogger().log(SentryLevel.DEBUG, "Found no UncaughtExceptionHandler to remove."); } - } else { + return; + } + + if (!visited.add(currentHandler)) { if (options != null) { - options.getLogger().log(SentryLevel.DEBUG, "Found no current handler to remove."); + options + .getLogger() + .log( + SentryLevel.WARNING, + "Cycle detected in UncaughtExceptionHandler chain while removing handler."); } return; } - if (currentHandler instanceof UncaughtExceptionHandlerIntegration) { - final UncaughtExceptionHandlerIntegration currentHandlerIntegration = - (UncaughtExceptionHandlerIntegration) currentHandler; - if (this == currentHandlerIntegration.defaultExceptionHandler) { - currentHandlerIntegration.defaultExceptionHandler = defaultExceptionHandler; + if (!(currentHandler instanceof UncaughtExceptionHandlerIntegration)) { + return; + } - if (options != null) { - options - .getLogger() - .log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed."); - } + final UncaughtExceptionHandlerIntegration currentHandlerIntegration = + (UncaughtExceptionHandlerIntegration) currentHandler; - } else { - removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler, visited); + if (this == currentHandlerIntegration.defaultExceptionHandler) { + currentHandlerIntegration.defaultExceptionHandler = defaultExceptionHandler; + if (options != null) { + options.getLogger().log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed."); } + } else { + removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler, visited); } } From 946c0e96ab949ac35c926030b0a541c25f8bf896 Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Fri, 11 Jul 2025 10:43:11 +0200 Subject: [PATCH 6/6] add test for cycle detection when trying to remove a handler --- ...UncaughtExceptionHandlerIntegrationTest.kt | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt index 2bff144824b..a958ba11c1e 100644 --- a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt @@ -12,6 +12,7 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.argWhere import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.atLeastOnce import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -467,4 +468,62 @@ class UncaughtExceptionHandlerIntegrationTest { assertEquals(integrations[2].get(), currentDefaultHandler) } + + @Test + fun `removeFromHandlerTree detects and handles cyclic dependencies`() { + var currentDefaultHandler: Thread.UncaughtExceptionHandler? = null + val scopes2 = mock() + val scopes3 = mock() + val scopes4 = mock() + + whenever(scopes2.globalScope).thenReturn(mock()) + whenever(scopes3.globalScope).thenReturn(mock()) + whenever(scopes4.globalScope).thenReturn(mock()) + + val handler = mock() + whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler } + + whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull())).then { + currentDefaultHandler = it.getArgument(0) + null + } + + val logger = mock() + val options = SentryOptions().apply { + setLogger(logger) + isDebug = true + } + + val handlerA = UncaughtExceptionHandlerIntegration(handler) + val handlerB = UncaughtExceptionHandlerIntegration(handler) + handlerA.register(fixture.scopes, options) + handlerB.register(scopes2, options) + + // Cycle: A → B → A + val defaultHandlerField = UncaughtExceptionHandlerIntegration::class.java.getDeclaredField("defaultExceptionHandler") + defaultHandlerField.isAccessible = true + defaultHandlerField.set(handlerA, handlerB) + defaultHandlerField.set(handlerB, handlerA) + + // Register handlerC to be removed from the chain + val handlerC = UncaughtExceptionHandlerIntegration(handler) + handlerC.register(scopes3, options) + + assertEquals(handlerC, currentDefaultHandler) + + // Register handlerD to be the current default + // Same Scope as handlerC so that removing handlerC would trigger a cycle + val handlerD = UncaughtExceptionHandlerIntegration(handler) + handlerD.register(scopes3, options) + + assertEquals(handlerD, currentDefaultHandler) + + handlerC.close() + + // Verify cycle detection warning was logged + verify(logger, atLeastOnce()).log( + SentryLevel.WARNING, + "Cycle detected in UncaughtExceptionHandler chain while removing handler." + ) + } }