-
-
Notifications
You must be signed in to change notification settings - Fork 459
feat(android-distribution): Run checkForUpdate on background thread (EME-413) #4811
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
base: main
Are you sure you want to change the base?
Conversation
…EME-413) Implement async version of checkForUpdate that runs on a background thread instead of blocking the calling thread. The callback is invoked on the background thread, allowing callers to dispatch to main thread if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
onResult.onResult(result) | ||
Thread { | ||
val result = checkForUpdateBlocking() | ||
onResult.onResult(result) |
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.
Potential bug: The onResult
callback is not wrapped in a try-catch block, which could lead to an app crash if the user's implementation throws an exception.
-
Description: The
onResult.onResult(result)
call is executed on a background thread without any exception handling. If the user-providedonResult
callback throws an exception, it will be uncaught on that thread, likely causing the entire application to crash. This is inconsistent with other parts of the SDK, such as the handling ofOnDiscardCallback
, where user-provided callbacks are defensively wrapped in atry-catch
block to prevent such failures. -
Suggested fix: Wrap the
onResult.onResult(result)
call within atry-catch (Throwable e)
block. Log any caught exceptions using the provided logger, similar to the pattern used for other user callbacks in the SDK to protect the app from crashing.
severity: 0.75, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
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.
This is a good point but we should just crash here.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f634d01 | 375.06 ms | 420.04 ms | 44.98 ms |
f634d01 | 359.58 ms | 433.88 ms | 74.30 ms |
23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
b3d8889 | 371.33 ms | 426.24 ms | 54.92 ms |
1df7eb6 | 397.04 ms | 429.64 ms | 32.60 ms |
ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
1df7eb6 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
Previous results on branch: no/eme-413-async-update-check
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9212043 | 373.06 ms | 450.63 ms | 77.57 ms |
1f983f6 | 324.46 ms | 452.26 ms | 127.80 ms |
b9cb481 | 381.29 ms | 429.46 ms | 48.17 ms |
b15d8fa | 351.88 ms | 417.20 ms | 65.32 ms |
15641a2 | 394.08 ms | 431.73 ms | 37.65 ms |
9ac0631 | 402.78 ms | 476.65 ms | 73.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9212043 | 1.58 MiB | 2.11 MiB | 539.71 KiB |
1f983f6 | 1.58 MiB | 2.11 MiB | 539.76 KiB |
b9cb481 | 1.58 MiB | 2.11 MiB | 539.76 KiB |
b15d8fa | 1.58 MiB | 2.11 MiB | 539.70 KiB |
15641a2 | 1.58 MiB | 2.11 MiB | 539.76 KiB |
9ac0631 | 1.58 MiB | 2.11 MiB | 539.70 KiB |
...android-distribution/src/main/java/io/sentry/android/distribution/DistributionIntegration.kt
Outdated
Show resolved
Hide resolved
…ync checkForUpdate (EME-413) Replace callback-based API with Future-based API to avoid confusion and improve consistency with SDK patterns. Use SentryExecutorService instead of spawning new threads to better manage resources. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…y (EME-413) Replace CompletableFuture.completedFuture() with a custom CompletedFuture implementation in NoOpDistributionApi to maintain Android API 21+ compatibility. CompletableFuture was only added in Android API 24. The new CompletedFuture follows the same pattern as existing CancelledFuture implementations in the codebase and returns an immediately completed Future with a result. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
...android-distribution/src/main/java/io/sentry/android/distribution/DistributionIntegration.kt
Outdated
Show resolved
Hide resolved
// TODO implement this in a async way | ||
val result = checkForUpdateBlocking() | ||
onResult.onResult(result) | ||
public override fun checkForUpdate(): java.util.concurrent.Future<UpdateStatus> { |
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.
also, I guess as a nice follow-up we could have distribution-ktx
that converts this to coroutines, but that's definitely not something we need to do now, if ever :)
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.
LGTM, thanks for using the executor service!
…EME-413) Add explicit import for java.util.concurrent.Future and use short form instead of fully qualified name in method signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…API (EME-413) Update sample app to use the new Future-based checkForUpdate() method instead of the old callback-based API. The Future is processed on a background thread and results are posted back to the UI thread. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improves code readability by importing Future instead of using fully qualified name. Adds guidance comment suggesting developers convert the sample to their preferred async library (RxJava, Coroutines, etc.) in production code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Applied spotlessApply formatting to wrap long comment line. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Refactors the async
checkForUpdate()
API to return aFuture<UpdateStatus>
instead of using a callback pattern, and usesSentryExecutorService
for background execution instead of spawning new threads.Changes
checkForUpdate(UpdateCallback)
withFuture
-basedcheckForUpdate()
SentryExecutorService
instead of creating new threads for each callUpdateCallback
interface entirelyBenefits
SentryExecutorService
#skip-changelog
Fixes EME-413
🤖 Generated with Claude Code