Dispatcher catches and rethrows Throwable instead of Exception to avoid swallowing errors #1894
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1894 +/- ##
=======================================
Coverage 82.25% 82.25%
=======================================
Files 227 227
Lines 7968 7968
Branches 1121 1121
=======================================
Hits 6554 6554
Misses 967 967
Partials 447 447 ☔ View full report in Codecov by Sentry. |
| @@ -65,7 +65,7 @@ internal open class Dispatcher( | |||
| val commandHandlingExceptions = Runnable { | |||
| try { | |||
| command.run() | |||
| } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { | |||
| } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) { | |||
There was a problem hiding this comment.
Great catch!!!
But yeah good thinking on those drawbacks... If we want to be safe, I think we could instead, catch the error and log it but not propagate it and maybe add a new Diagnostics event for these Throwable + not Exception errors.
On the other hand, I believe this is used only in a few places, so maybe we can fix those so we just propagate the Throwable as a "normal" exception, even if it's not an exception that we can propagate through the normal error channels... Wdyt?
There was a problem hiding this comment.
Yea I think we have 2 choices:
- Log an error, but don't rethrow the
Throwable. The app would still hang, but it's better than silently hanging. - Rethrow the
Throwable. The app would crash, but that means the chance is higher we come to know of it and get to fix it.
I don't know what we prefer in terms of developer experience. Option 1 might leave the developer confused, unless they happen to see the error log. Option 2 might leave them annoyed that the app crashed.
I'm slightly leaning towards option 2, but I'm okay with 1 too. @tonidero @vegaro what do you think?
There was a problem hiding this comment.
right, so I think if we want to minify risk, we basically would need to make sure we catch any Throwable's that we weren't caching before and map them to exceptions we can display through the API.
Admittedly that's more work, but probably the safest... Having said that, I wouldn't expect this to uncover many issues and if it does, there is probably something very wrong that wasn't working, so maybe it's ok to just do option 2 (which is what's implemented in this PR). I'm ok with this option as well.
There was a problem hiding this comment.
I am down to go with option 2. If an app starts to crash is very likely due to a bug in our code that we need to be able fix.
I remember I fixed a very similar issue in the past where exceptions were getting swallowed and it helped us uncover very important issues in our code
There was a problem hiding this comment.
Alright, since we're all okay with option 2, I'd say let's go that route. That indeed maximizes visibility of any potential issues this might uncover.
I have marked the PR as ready for review.
**This is an automatic release.** ## RevenueCat SDK ### ✨ New Features * Add `tenjinAnalyticsInstallationId` setter property (#1897) via Toni Rico (@tonidero) ### 🐞 Bugfixes * [Fix] Consider a network error as not successfully synced for paywall events (#1900) via Mark Villacampa (@MarkVillacampa) ### 🔄 Other Changes * [Paywalls] Synchronize paywall events on app backgrounding and after a purchase (#1901) via Mark Villacampa (@MarkVillacampa) * Dispatcher catches and rethrows Throwable instead of Exception to avoid swallowing errors (#1894) via JayShortway (@JayShortway) --------- Co-authored-by: revenuecat-ops <[email protected]> Co-authored-by: Toni Rico <[email protected]>
Note: this PR is intended as more of a discussion starter, which is why it's a draft.
Issue
We sometimes come across issues where the SDK seems to hang. This is annoying to investigate, as no error is logged (nor does it crash). See e.g.:
Cause
I found one cause of hanging while investigating the second issue. Errors can be swallowed by the
Dispatcher, as it only rethrowsExceptions. However, not all errors areExceptions. An example of an error which is aThrowablebut not anExceptionis aNoSuchMethodError.When such an error happens in an async call and is not caught, the thread dies before the callback can be called, causing the async call to hang.
Fix
To truly surface all issues, we should rethrow
Throwableinstead.Discussion
Of course, there is a risk to this, as we might now surface errors that were swallowed before, causing the SDK to crash in new scenarios (where it would likely hang before). I'd like to know your thoughts on this, and how you think we should mitigate.