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.

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 2 commits into
base: main
Choose a base branch
from

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented May 28, 2025

📜 Description

Reworks registration and removal of UncaughtExceptionHandlerIntegration to be able to add one UncaughtExceptionHandlerIntegration per scopes.globalScope.
Functionality is now consistent with the documentation about using Sentry in an SDK here.

💡 Motivation and Context

Fixes #4429

💚 How did you test it?

  • Added new automated tests

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link
Contributor

github-actions bot commented May 28, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 444.56 ms 544.92 ms 100.36 ms
Size 1.58 MiB 2.08 MiB 511.00 KiB

@lbloder lbloder marked this pull request as ready for review May 28, 2025 14:45
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

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

one question, but LGTM otherwise, nice job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UncaughtExceptionHandlerIntegration not working correctly when registered against multiple IHub/Scopes
2 participants