Use lifecycleScope where possible in Stripe.kt #3567
Conversation
- Use `lifecycleScope` where possible. - Make some `PaymentController` methods `suspend`. This will help with potential memory leaks caused by `StripePaymentController`. - Inject `uiContext` to `StripePaymentController`.
mshafrir-stripe
requested review from
ccen-stripe,
brnunes-stripe,
ebarrenechea-stripe and
michelleb-stripe
| activity: Activity | ||
| ): CoroutineScope = when (activity) { | ||
| is ComponentActivity -> activity.lifecycleScope | ||
| else -> MainScope() |
There was a problem hiding this comment.
I'm not sure if this else branch is an edge case. MainScope is a very sensible fallback here but it lacks knowledge about the component's lifecycle. The coroutine will continue to execute (and hold a strong reference to the activity) even after the activity finishes. I think a lifecycle observer wrapper would be a good option here but that will also depend on min SDK version and/or if we're using the androidx libraries. At the end of the day, I feel like this might be just a poor back-port of the ComponentActivity approach. Is there any data on how many of our users are still using the plain Activity vs its support lib/androidx counterparts?
In any case, MainScope will do fine here but might result in leaking activities in apps using old code.
There was a problem hiding this comment.
Yeah, this isn't ideal and mainly an attempt to make things better for users using ComponentActivity subclasses without introducing a breaking API change (will consider doing that in a future PR).
It might actually make sense to use Dispatchers.IO as the fallback, which is the existing behavior.
| @@ -73,7 +73,8 @@ internal class StripePaymentController internal constructor( | |||
| private val paymentRelayLauncher: ActivityResultLauncher<PaymentRelayStarter.Args>? = null, | |||
| private val paymentAuthWebViewLauncher: ActivityResultLauncher<PaymentAuthWebViewContract.Args>? = null, | |||
| private val stripe3ds2ChallengeLauncher: ActivityResultLauncher<PaymentFlowResult.Unvalidated>? = null, | |||
| private val workContext: CoroutineContext = Dispatchers.IO | |||
| private val workContext: CoroutineContext = Dispatchers.IO, | |||
There was a problem hiding this comment.
Do we need to specify a work context here?Could we get away with just specifying the ui context and delegating actual context switch to the components implementing the suspend functions?
Edit: I had a quick look at the code for some of our dependencies and we're injecting the work context into DefaultStripeChallengeStatusReceiver. This seems like the only place where we would "need" a work context. There are ways around this requirement, though and maybe something we can address in a different PR.
There was a problem hiding this comment.
I think you're right. I'll take a look at this in a follow-up PR.
stripe/src/main/java/com/stripe/android/StripePaymentController.kt
Outdated
Show resolved
Hide resolved
Summary
lifecycleScopewhere possible.PaymentControllermethodssuspend. This will help withpotential memory leaks caused by
StripePaymentController.uiContexttoStripePaymentController.Motivation
Make
StripePaymentControllerlifecycle-aware.Testing