-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
Fix missing null check when removing lifecycle observer #2625
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d81684e | 235.73 ms | 328.76 ms | 93.03 ms |
17ab223 | 427.65 ms | 484.31 ms | 56.65 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d81684e | 1.73 MiB | 2.26 MiB | 547.78 KiB |
17ab223 | 1.73 MiB | 2.34 MiB | 626.85 KiB |
Previous results on branch: fix/npe-sentry-close-lifecycle-watcher
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c8d5fe8 | 377.53 ms | 442.44 ms | 64.91 ms |
49c4a8c | 350.21 ms | 380.89 ms | 30.68 ms |
456e642 | 356.06 ms | 368.73 ms | 12.67 ms |
f1307d7 | 362.40 ms | 397.60 ms | 35.20 ms |
a7e3859 | 314.22 ms | 378.35 ms | 64.13 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c8d5fe8 | 1.73 MiB | 2.26 MiB | 548.12 KiB |
49c4a8c | 1.73 MiB | 2.34 MiB | 626.94 KiB |
456e642 | 1.73 MiB | 2.26 MiB | 548.12 KiB |
f1307d7 | 1.73 MiB | 2.26 MiB | 548.03 KiB |
a7e3859 | 1.73 MiB | 2.26 MiB | 548.12 KiB |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #2625 +/- ##
=========================================
Coverage 81.37% 81.37%
Complexity 4219 4219
=========================================
Files 337 337
Lines 15599 15599
Branches 2037 2037
=========================================
Hits 12693 12693
Misses 2111 2111
Partials 795 795 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test to verify the change? like verifying the watcher is not null after the close
function is called in a background thread and it's null only after the main thread executes?
sentry-android-core/src/main/java/io/sentry/android/core/AppLifecycleIntegration.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a minor comment, if possible.
Looks good for everything else!
}.start() | ||
|
||
latch.await() | ||
InstrumentationRegistry.getInstrumentation().waitForIdleSync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the main looper it should be possible to use shadowOf(Looper.getMainLooper()).idle()
instead of creating a thread manually. It would be easier for readability if it works
📜 Description
If
Sentry.close
is called off the main thread, the lifecycle watcher field is set tonull
, while de-registering is posted on the main thread, leading to a NPE in combination with some versions of android x lifecycle lib.💡 Motivation and Context
Fixes #2601
💚 How did you test it?
Manual testing, the overhead of running a single test against different version of the androidx lifecycle library didn't seem to justify the efforts.
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps