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

Merged
lbloder merged 7 commits into from
Jul 11, 2025
+310 −22
Merged

Allow multiple UncaughtExceptionHandlerIntegrations to be active #4462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 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
f9a5cab
add initial cycle detection
lbloder Jun 30, 2025
db374ab
make recursive method more readable
lbloder Jun 30, 2025
946c0e9
add test for cycle detection when trying to remove a handler
lbloder Jul 11, 2025
a21f66d
Merge branch 'main' into fix/multiple-uncaught-exception-handlers
lbloder Jul 11, 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.17.0

### Features
Expand Down
127 changes: 106 additions & 21 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,9 +10,12 @@
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;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
Expand All @@ -28,6 +31,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 +70,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

@romtsn romtsn Jun 2, 2025

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

@lbloder lbloder Jun 3, 2025

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

@romtsn romtsn Jun 3, 2025

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

@lbloder lbloder Jun 3, 2025

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

@adinauer adinauer Jun 24, 2025

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

@romtsn romtsn Jun 24, 2025

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,14 +168,88 @@ 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());
}
}
}

/**
Copy link
Member

@adinauer adinauer Jun 24, 2025

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.

* 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.
*
* <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, 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,
@NotNull Set<Thread.UncaughtExceptionHandler> visited) {

if (currentHandler == null) {
if (options != null) {
options.getLogger().log(SentryLevel.DEBUG, "Found no UncaughtExceptionHandler to remove.");
}
return;
}

if (!visited.add(currentHandler)) {
if (options != null) {
options
.getLogger()
.log(
SentryLevel.WARNING,
"Cycle detected in UncaughtExceptionHandler chain while removing handler.");
}
return;
}

if (!(currentHandler instanceof UncaughtExceptionHandlerIntegration)) {
return;
}

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, visited);
}
}

Expand Down
Loading
Loading