Allow multiple UncaughtExceptionHandlerIntegrations to be active

[lbloder]

📜 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

[github-actions]

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

[romtsn]

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?

[lbloder]

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?

[romtsn]

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

[lbloder]

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?

[adinauer]

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/

[romtsn]

yeah but that doc uses Scopes

[romtsn]

one question, but LGTM otherwise, nice job!

[adinauer]

LGTM, one question regarding circular reference

[adinauer]

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

[adinauer]

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

[lbloder]

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?

[adinauer]

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