Call Sentry#close on JVM shutdown.
#497
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
| @@ -115,6 +116,14 @@ private static void installDefaultIntegrations( | |||
| final @NotNull IBuildInfoProvider buildInfoProvider, | |||
| final @NotNull ILoadClass loadClass) { | |||
|
|
|||
| options.addIntegration( | |||
There was a problem hiding this comment.
As discussed offline, for now it makes sense to just add the "event cache" related stuff into Android and focus on the core not having offline caching altogether.
Down the road when the need comes we can revisit and think of a better way to "opt-in" to offline mode in a simple way, that would work for Android or Java apps (say desktop could leverage that)
There was a problem hiding this comment.
👍 Just so that it doesn't get lost - I think we should expose ideally single method that turns on caching which takes cachePathDir as parameter and under the hood also turns on all cache-related integrations.
sentry-core/src/main/java/io/sentry/core/ShutdownHookIntegration.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/ShutdownHookIntegration.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/ShutdownHookIntegration.java
Outdated
Show resolved
Hide resolved
- close Hub instead of Sentry - pass Runtime as constructor parameter in ShutdownHookIntegration to make class testable
|
Thanks @marandaneto for a review. Fixes are applied. Regarding target branch, my understanding is that everything that affects Sentry Core goes to master and only other framework-specific integrations are meant to go to |
| } | ||
|
|
||
| @Override | ||
| public void register(@NotNull IHub hub, @NotNull SentryOptions options) { |
There was a problem hiding this comment.
| public void register(@NotNull IHub hub, @NotNull SentryOptions options) { | |
| public void register(final @NotNull IHub hub, final @NotNull SentryOptions options) { |
There was a problem hiding this comment.
any reason to not apply the final modifier?
There was a problem hiding this comment.
No specific reason, I've seen some methods with final and some without. I'll add them.
My personal take on final on method parameters is that they clutter code and instead all parameters should be treated as final implicitly. That's just an opinion, I will adjust to existing coding standards.
There was a problem hiding this comment.
got it, just asking cus it was in my first GH "suggestion".
Agreed that they should be treated as final implicitly, but IMO when you have multithread systems, having an extra final makes the compiler to help you out if you are not cloning/deep copying an object but instead messing up with the references, we had a bug like this a few months ago, so we've decided to add in all the missing ones/and new ones
There was a problem hiding this comment.
Perhaps there is a way to configure static code analysis to make sure it doesn't sneak in?
There was a problem hiding this comment.
all the data classes are mutable by design, I've tried once but then it warned about almost everything.
so basically what I am doing is, new things I always try to make everything final if it can and if I am touching some classes, I already check the missing final and @NotNull, @Nullable to make better interoperability with Kotlin tests
I see, that makes sense, I'm just wondering if the |
|
I didn't find any documentation stating that shutdown hooks behave differently on Android but I haven't tested it and I also don't think I am the right person to do it as my Android experience is limited (I can though it will just take time). I'll leave it up to you and @bruno-garcia to decide how to proceed. |
let's merge then and I test it, last case we just remove adding this integration by default, the integration itself is already a big gain. |
📢 Type of change
📜 Description
Sentry#closeon JVM shutdown - as a result flush all Sentry events that are in progress.SendCachedEventFireAndForgetIntegrationby default only for Android as for the console/server application cache does not apply💡 Motivation and Context
Without this change, when Sentry is used in the console application, the main thread may finish before events are sent to Sentry and as a result no events are sent.
This idea was briefly discussed with @marandaneto and it's a subject for discussion - perhaps there is a better way to achieve the same result without attaching shutdown hook.
💚 How did you test it?
📝 Checklist
🔮 Next steps