Possible bug ?

[anmol-tray]

purchases-android/purchases/src/defaults/kotlin/com/revenuecat/purchases/coroutinesExtensions.kt

Line 24 in 36ddd19

return suspendCoroutine { continuation ->

it should be suspendCancellableCoroutine instead of suspendCoroutine

[RCGitBot]

👀 We've just linked this issue to our internal tracker and notified the team. Thank you for reporting, we're checking this out!

[rglanz-rc]

Hi @anmol-tray could you give some more context on why you believe this is a bug? It seems that the cancellation handling of suspendCancellableCoroutine could be an enhancement, but are you running into problems with this code as is?

[oiatomic]

Hi @anmol-tray could you give some more context on why you believe this is a bug? It seems that the cancellation handling of suspendCancellableCoroutine could be an enhancement, but are you running into problems with this code as is?

Yes because when we cancel a coroutine using a job.cancel the suspendCoroutine doesn't cancel the revenueCat call.. but wrapping the normal non suspend function for get current customer in my own suspendCancellableCoroutine cancells the coroutine.

Ref:

android/codelab-kotlin-coroutines#8

https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/suspend-cancellable-coroutine.html

It's important that the coroutine cancels properly with cancellation exception

If you think it's not an issue please feel free to close this issue.

Thanks!

[JayShortway]

Hi, thanks for raising this!

The reason we're not using suspendCancellableCoroutine, is because the underlying callback-style API is not cancellable. So even if the Job would be cancelled, the underlying call is still happening. Using suspendCancellableCoroutine we'd be hiding that fact, as it might seem that the underlying call is also being cancelled.

Do you have any specific reproducer you could share, that would be fixed by using suspendCancellableCoroutine?

[IlyaPavlovskii]

Hi, thanks for raising this!

The reason we're not using suspendCancellableCoroutine, is because the underlying callback-style API is not cancellable. So even if the Job would be cancelled, the underlying call is still happening. Using suspendCancellableCoroutine we'd be hiding that fact, as it might seem that the underlying call is also being cancelled.

Do you have any specific reproducer you could share, that would be fixed by using suspendCancellableCoroutine?

Yes, when I switch between users and try to synchronize purchases, I might receive a fallback for prev. user request. That's why we need support cancellation operation for that case.

As an alternative, I might prepare not replacing all suspend function, just **add new ** cancellable functions
@JayShortway What do you think?

UPD. @JayShortway PR updated

[JayShortway]

Hi @IlyaPavlovskii, thanks for providing more details. Can you share some code in a gist or repository that reproduces this behavior?

Yes, when I switch between users and try to synchronize purchases, I might receive a fallback for prev. user request. That's why we need support cancellation operation for that case.

That way we can better analyze the problem and determine the root cause.

[IlyaPavlovskii]

Hi @IlyaPavlovskii, thanks for providing more details. Can you share some code in a gist or repository that reproduces this behavior?

Yes, when I switch between users and try to synchronize purchases, I might receive a fallback for prev. user request. That's why we need support cancellation operation for that case.

That way we can better analyze the problem and determine the root cause.

Something like this:

fun authState(): Flow<FirebaseUser?> {
	val firebaseAuth = FirebaseAuth.getInstance()
	return callbackFlow {
		val callback = FirebaseAuth.AuthStateListener { firebaseAuth ->
			this.trySendBlocking(firebaseAuth.currentUser)
		}
		firebaseAuth.addAuthStateListener(callback)
		awaitClose {
			firebaseAuth.removeAuthStateListener(callback)
		}
	}
}

fun syncrhonization() {
	var syncJob: Job? = null
	val coroutineScope: CoroutineScope = MainScope()
	authState()
		.collectLatest { firebaseUser ->
			val purchases = Purchases.sharedInstance
			if (firebaseUser != null) {
				with(purchases) {
					this.awaitCancellableLogIn(firebaseUser.uid)
					this.setEmail(firebaseUser.email)
					this.setDisplayName(firebaseUser.displayName)
					this.setDisplayName(firebaseUser.displayName)
				}
				syncJob?.cancel()
				syncJob = coroutineScope.async {
					purchases.awaitCancellableRestore()
				}
				
			} else {
				syncJob?.cancel()
				if (!purchases.isAnonymous) {
					purchases.awaitCancellableLogOut()
				}
			}
		}.launchIn(coroutineScope)
}

[JayShortway]

Where are you getting a callback for a previous user? Is FirebaseAuth.AuthStateListener misbehaving? I see you're ignoring the result of awaitRestore().

It would be great if you could create a git repository with a failing unit test.

[IlyaPavlovskii]

Where are you getting a callback for a previous user? Is FirebaseAuth.AuthStateListener misbehaving? I see you're ignoring the result of awaitRestore().

It would be great if you could create a git repository with a failing unit test.

It might be catched in realtime with real network communication(we doesn't support UnitTesting with real remove communication).

There is no any problem with authStateListener.
Steps to reproduce problem:

1. Login to the system(await firebaseAuth state changed to the authorized state)
2. Call Purchases.sharedInstance.awaitLogIn(firebaseUser.uid)
3. Start to restore purchases in a async scope:

syncJob = coroutineScope.async {
    Purchases.sharedInstance.awaitRestore()
}

4.1 Logout immediately after this.

In the logout section I terminate the scope(syncJob?.cancel()), but the awaitRestore() function is not cancellable. That mean, that we should await execution result even if we don't need it anymore. Then I might receive the callback into the coroutineScope.async {purchases.awaitRestore()}, process it with previously logged in user data, even if I already logged out from firebase and only after that we might call Purchases.sharedInstance.awaitLogOut()
4.2 Logout immediately and login into the new account:
Now I might receive callback from firstly logged in user when we already logged in into the 2nd account.

[IlyaPavlovskii]

Try to reproduce it with simulation authorization and check behavior with 3 buttons: simulate_login_account_A, simulate_login_account_B, logout

[JayShortway]

Thanks for providing more info! I have a few follow-up questions:

1. May I ask why you are restoring programmatically on login? In general, it is recommended to restore only when the user clicks a “restore” button. Also, since you have your own user system (using Firebase Auth), it should be sufficient to log in with the correct appUserId, but maybe your setup is different?
2. How do you have the purchase transfer settings set up in the RevenueCat dashboard? (See Project Settings -> General)
3. Are you processing the resulting CustomerInfo from the restore? You could use the App User Id to check if it belongs to the currently logged in user.

[IlyaPavlovskii]

Thanks for providing more info! I have a few follow-up questions:

1. May I ask why you are restoring programmatically on login? In general, it is recommended to restore only when the user clicks a “restore” button. Also, since you have your own user system (using Firebase Auth), it should be sufficient to log in with the correct appUserId, but maybe your setup is different?
2. How do you have the purchase transfer settings set up in the RevenueCat dashboard? (See Project Settings -> General)
3. Are you processing the resulting CustomerInfo from the restore? You could use the App User Id to check if it belongs to the currently logged in user.

1. It's a personal need to restore previous purchases quickly(will update it to restore purchase button). Is it still a recommendation or requirement? Probably that clarification related to another problem that mentioned here - https://community.revenuecat.com/sdks-51/billingwrapper-is-not-attached-to-a-listener-after-relogin-4918, not related to cancellable wrapping.
2. We use the standard type: Transfer to new App USER ID
3. Yes, I'm trying to process it, but that PR is a proposal on how to avoid useless responses when suspend function is already completed/canceled/not-interested and I want to cancel it, without additional wrappers.

[JayShortway]

Restoring via a button is a recommendation, because it often leads to unexpected results if done programmatically. In your scenario, there's a Google account that bought the subscription, and multiple users logging in and out. Only the last user to log in will have access to the subscription of the Google account, because the purchase gets transferred upon restore.

Regarding the PR, we have discussed it within the team and decided that it is best if our async methods reflect the underlying work as much as possible. That is, since the underlying async work is not directly cancellable, the async method kicking off that work shouldn't be either. To illustrate, if you start a restore wrapped in suspendCancellableCoroutine, and then cancel the coroutine, the restore continues in the background. This means that the customer info will be updated, the entitlements and purchases are transferred to the new user, and the updatedCustomerInfoListener will be fired. It is unexpected that all these side effects happen when you have "cancelled" the restore. To avoid this confusion, it is best if our async methods truly reflect the underlying work, which is not cancellable.

We recommend using the App User ID to determine if you need to process the current CustomerInfo. Of course, if you want to call async methods in a fire-and-forget way, you can use the non-suspending variants and pass in an empty callback.

Happy to answer any other questions!