Skip to content

Allow multiple UncaughtExceptionHandlerIntegrations to be active #4462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+208 −24
Open

Allow multiple UncaughtExceptionHandlerIntegrations to be active #4462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c93c16
allow multiple UncaughtExceptionHandlerIntegrations to be active. one…
lbloder May 28, 2025
bcf3f84
Add changelog entry
lbloder May 28, 2025
0f1f0f4
add comments to close and removeFromHandlerTree methods
lbloder Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
101 changes: 78 additions & 23 deletions sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, any specific reason we limit this to globalScope only? Shouldn't we compare scopes == currentHandlerIntegration.scopes given that the integration works on the scopes level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was to allow exactly one UncaughtExceptionHandlerIntegration to be registered per Sentry instance. For that I used the globalScope as it is never forked.

With the new close logic, however, I think we could also do what you suggested and use scopes instead. But I'd have to test how this behaves

Do you see any pros/cons for one over the other?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much pros for now, but if we change how Scopes behave under-the-hood this may break in theory. And also "per Sentry instance" probably implies Scopes rather than globalScope. I ran the test locally after changing this condition to comparing scopes and it still passed. So, up to you :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I get your point regarding the inner workings of Scopes. I basically tried to be pretty conservative here as to not cause a regression of #3266.

In theory, with your suggestions, one can register multiple UncaughtExceptionHandlerIntegration by passing a forked Scopes instance to the register method. Then again, if all UncaughtExceptionHandlerIntegration instances are passed into the integration list of SentryOptions as per the documentation, they should still clean up nicely.

val integration = UncaughtExceptionHandlerIntegration()
integration.register(scopes, options)

val forkedScopes = scopes.forkedScopes("test")
val integration2 = UncaughtExceptionHandlerIntegration()
integration2.register(forkedScopes, options)

This will register two UncaughtExceptionHandlerIntegration instances, because by forking the Scopes we get a new Scopes instance.

If the integrations are added to the Sentry options:

options.addIntegration(integration)
options.addIntegration(integration2)

Then closing either the original or forked scopes will close both UncaughtExceptionHandlerIntegration instances.

I'm fine with both approaches. I think the question becomes whether we want to allow that behaviour. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be careful with SDK re-init scenarios here where we could end up regressing on #3266 and breaking the fix in #3398.

I'd err on the side of caution and only have one per global scope as currently implemented in this PR.

If the use case for having multiple instances of UncaughtExceptionHandlerIntegration active is separate instances of Sentry then in theory only one of them should be using static API and the other should be set up with a separate global scope as documented in https://docs.sentry.io/platforms/android/configuration/shared-environments/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but that doc uses Scopes

defaultExceptionHandler = currentHandlerIntegration.defaultExceptionHandler;
} else {
defaultExceptionHandler = currentHandler;
}
} else {
defaultExceptionHandler = currentHandler;
}
}
}

threadAdapter.setDefaultUncaughtExceptionHandler(this);
threadAdapter.setDefaultUncaughtExceptionHandler(this);
}

this.options
.getLogger()
Expand Down Expand Up @@ -157,13 +166,59 @@ static Throwable getUnhandledThrowable(
return new ExceptionMechanismException(mechanism, thrown, thread);
}

/**
* Remove this UncaughtExceptionHandlerIntegration from the exception handler chain.
*
* <p>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() {
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.");
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the comments, makes it a lot easier to understand what it's doing.

* Recursively traverses the chain of UncaughtExceptionHandlerIntegrations to find and remove this
* specific integration instance.
*
* <p>Checks if this instance is the defaultExceptionHandler of the current handler, if so replace
* with its own defaultExceptionHandler, thus removing it from the chain.
*
* <p>If not, recursively calls itself on the next handler in
* the chain.
*
* <p>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 =
(UncaughtExceptionHandlerIntegration) currentHandler;
if (this == currentHandlerIntegration.defaultExceptionHandler) {
currentHandlerIntegration.defaultExceptionHandler = defaultExceptionHandler;

if (options != null) {
options
.getLogger()
.log(SentryLevel.DEBUG, "UncaughtExceptionHandlerIntegration removed.");
}

} else {
removeFromHandlerTree(currentHandlerIntegration.defaultExceptionHandler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to end up with infinite recursion here due to circular reference between two Sentry handlers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good question, theoretically yes if there is a circular reference between the handlers. Not sure if there is a scenario in which a cyclic dependency can happen. Need to play this through. We could add safety measures though to make sure we only visit each uncaught exception handler once. Or we define a max depth for the recursion. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you find easier to implement should work fine I think.

}
}
}
Expand Down
125 changes: 124 additions & 1 deletion sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -344,4 +346,125 @@ class UncaughtExceptionHandlerIntegrationTest {

assertEquals(initialUncaughtExceptionHandler, currentDefaultHandler)
}

@Test
fun `multiple registrations with different global scopes allowed`() {
val scopes2 = mock<IScopes>()
val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> }

var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler

val handler = mock<UncaughtExceptionHandler>()
whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler }

whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull<Thread.UncaughtExceptionHandler>())).then {
currentDefaultHandler = it.getArgument(0)
null
}

whenever(scopes2.globalScope).thenReturn(mock<IScope>())

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<IScopes>()
val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> }

var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler

val handler = mock<UncaughtExceptionHandler>()
whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler }

whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull<Thread.UncaughtExceptionHandler>())).then {
currentDefaultHandler = it.getArgument(0)
null
}

whenever(scopes2.globalScope).thenReturn(mock<IScope>())

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<IScopes>()
val scopes3 = mock<IScopes>()
val scopes4 = mock<IScopes>()
val scopes5 = mock<IScopes>()

val scopesList = listOf(fixture.scopes, scopes2, scopes3, scopes4, scopes5)

val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> }

var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler

val handler = mock<UncaughtExceptionHandler>()
whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler }

whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull<Thread.UncaughtExceptionHandler>())).then {
currentDefaultHandler = it.getArgument(0)
null
}

whenever(scopes2.globalScope).thenReturn(mock<IScope>())
whenever(scopes3.globalScope).thenReturn(mock<IScope>())
whenever(scopes4.globalScope).thenReturn(mock<IScope>())
whenever(scopes5.globalScope).thenReturn(mock<IScope>())

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)
}
}