Migrate all suspendCoroutine usages to suspendCancellableCoroutine#3365
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3365 +/- ##
==========================================
+ Coverage 79.20% 79.34% +0.13%
==========================================
Files 360 361 +1
Lines 14437 14441 +4
Branches 1982 1960 -22
==========================================
+ Hits 11435 11458 +23
+ Misses 2194 2190 -4
+ Partials 808 793 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tonidero
left a comment
There was a problem hiding this comment.
Looking great! Just some thoughts but it's great to have this. Thank you!!
| return false | ||
| } | ||
| return suspendCoroutine { continuation -> | ||
| return suspendCancellableCoroutine { continuation -> |
There was a problem hiding this comment.
I think the main reason we didn't do this originally is that this is part of the coroutines library, which we didn't originally add. However, we have since added it so it makes sense to do this 🙌 Thank you!!
| return false | ||
| } | ||
| return suspendCoroutine { continuation -> | ||
| return suspendCancellableCoroutine { continuation -> |
There was a problem hiding this comment.
Another point to note is that these are not really cancellable... In fact, we're just ignoring the result value but not cancelling the request. I'm not sure if it would be better to keep it as suspendCoroutine to better reflect what's happening... Having said that, I do understand using this has some benefits so not opposed either. cc @JayShortway for thoughts 🙏
There was a problem hiding this comment.
Yea, we've had this contributed in the past, but indeed the underlying operation is not cancellable. We're not using continuation.invokeOnCancellation now either, which is what suspendCancellableCoroutine would enable.
Happy to hear other benefits this might bring if I'm missing something!
There was a problem hiding this comment.
Yea, you're right that the underlying operations aren't truly cancellable we're not cancelling the actual requests, just ignoring the result.
So the main motivation is coroutine lifecycle management (like more higher-level usages): with suspendCoroutine, if the calling scope is cancelled (e.g., ViewModel cleared), the coroutine hangs forever. suspendCancellableCoroutine lets it unblock immediately and clean up captured references. So callers can rely on standard cancellation (viewModelScope, withTimeout, etc.) working correctly with our await* APIs.
Adding invokeOnCancellation to actually cancel underlying requests could be a nice follow-up!
| billingResult.responseCode == BillingClient.BillingResponseCode.OK, | ||
| ) | ||
| if (continuation.isActive) { | ||
| continuation.resume( |
There was a problem hiding this comment.
Since we're doing this so many times, I wonder if it would be worth to create a safeResume and safeResumeWithException functions that check whether the coroutine is active... Then, we could write a custom lint rule to disallow the "non-safe" versions, at least for now. That way, it would be more difficult to forget to add this. Wdyt?
There was a problem hiding this comment.
Thanks for your suggestion!
I implemented safeResume / safeResumeWithException and used at every suspendCancellableCoroutine site in this PR.
One note: the helpers are internal in :purchases, which forced a near-duplicate copy in :ui:revenuecatui. If we're adding a lint rule anyway, worth promoting to @InternalRevenueCatAPI and deduping at the same time. I can do it here or in the follow-up PR.
There was a problem hiding this comment.
Great point... I think that can be part of another PR indeed. Thanks for adding this!
tonidero
left a comment
There was a problem hiding this comment.
While I think the benefits are small considering the operation itself won't actually cancel, I think in the end it's a net benefit and shouldn't have any drawbacks that I could think of, so let's go with this. Thank you!!
**This is an automatic release.** ## RevenueCat SDK ### ✨ New Features * Unified StoreReplacementMode API (RevenueCat#3234) via Will Taylor (@fire-at-will) * Add placement and targeting context to paywall events (RevenueCat#3253) via Dan Pannasch (@dpannasch) ### 🐞 Bugfixes * Fix null Placements when offering_ids_by_placement is absent (RevenueCat#3254) via Dan Pannasch (@dpannasch) ## RevenueCatUI SDK ### Paywallv2 #### ✨ New Features * Wire multipage workflow navigation into PaywallViewModel (RevenueCat#3381) via Cesar de la Vega (@vegaro) ### 🔄 Other Changes * Add `triggerType` to `WorkflowTrigger` (RevenueCat#3393) via Cesar de la Vega (@vegaro) * Extract private function `NavigateTo.toPaywallAction` (RevenueCat#3392) via Cesar de la Vega (@vegaro) * Bump revenucatui-tests gradle cache key (RevenueCat#3391) via Toni Rico (@tonidero) * Create `WorkflowTriggerType` and `WorkflowTriggerActionType` (RevenueCat#3386) via Cesar de la Vega (@vegaro) * Update baseline profiles (RevenueCat#3390) via RevenueCat Git Bot (@RCGitBot) * Plumb `componentId` through buttons on workflow interactions (RevenueCat#3380) via Cesar de la Vega (@vegaro) * Add `ButtonComponent.Action.Workflow` (RevenueCat#3385) via Cesar de la Vega (@vegaro) * Add `componentId` to `ButtonCoomponentStyle` (RevenueCat#3384) via Cesar de la Vega (@vegaro) * Migrate all suspendCoroutine usages to suspendCancellableCoroutine (RevenueCat#3365) via Jaewoong Eum (@skydoves) * Add `WorkflowNavigator` for multipage workflow step navigation (RevenueCat#3379) via Cesar de la Vega (@vegaro) * build(deps): bump fastlane-plugin-revenuecat_internal from `b822f01` to `d24ab26` (RevenueCat#3383) via dependabot[bot] (@dependabot[bot]) * Add `id` field to `ButtonComponent` (RevenueCat#3377) via Cesar de la Vega (@vegaro) * Add CI workflows for generating Baseline Profiles (RevenueCat#3372) via Jaewoong Eum (@skydoves) * add min sdk level for paywalls and customer center (RevenueCat#2465) via Muhammad-Sharif Moustafa (@mshmoustafa) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk release bookkeeping: updates version strings, changelogs, and documentation deployment targets without changing runtime logic beyond the reported version identifier. > > **Overview** > Cuts the `10.3.0` release by updating all version references from `10.3.0-SNAPSHOT` to `10.3.0` across build config (`gradle.properties`, `.version`, `Config.frameworkVersion`) and sample/test app dependency pins. > > Updates release documentation publishing to sync Dokka output to the `10.3.0` S3 path and changes `docs/index.html` to redirect to `10.3.0`. Also promotes release notes by moving `CHANGELOG.latest.md` entries into a new `CHANGELOG.md` section for `10.3.0`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 056ce62. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Migrate all
suspendCoroutineusages tosuspendCancellableCoroutinewithisActiveguards.Why: suspendCoroutine is not cancellation-aware, which means if the calling coroutine scope is cancelled (e.g., ViewModel cleared), the coroutine hangs forever instead of being cleaned up. This causes silent resource leaks and violates structured concurrency.
What changed:
continuation.isActivechecks before everyresume/resumeWithExceptionto preventIllegalStateExceptionwhen callbacks fire after cancellationNote
Medium Risk
Touches many public coroutine
await*APIs and purchase/UI flows; cancellation now propagates (instead of potentially hanging), which is a behavior change that could affect callers with broad exception handling.Overview
Migrates coroutine wrappers across the SDK and RevenueCatUI from
suspendCoroutinetosuspendCancellableCoroutineand adds guarded resumption (safeResume/safeResumeWithExceptionorisActivechecks) to preventIllegalStateExceptionwhen callbacks fire multiple times or after cancellation.Updates internal helpers (e.g.,
IdentityManager.aliasCurrentUserIdTo, Blockstore calls) and example BillingClient purchase/ack/consume flows to be cancellation-aware, and adds new cancellation-focused tests for keyawait*functions. The changelog notes the behavior change thatCancellationExceptionnow propagates from publicawait*APIs.Reviewed by Cursor Bugbot for commit 04f97e6. Bugbot is set up for automated code reviews on this repo. Configure here.