Fixed Coroutine Cancellation Exception for concurrent close and offer

[Daeda88]

No description provided.

[nbransby]

What was the error you were encounting on which platform?

[Daeda88]

Its a complicated stack, but short summary:

Preface:

• I have a Kotlin MPP viewModel that can navigate to another Kotlin MPP ViewModel
• The ViewModels run a coroutineScope that is cancelled whenever the View bound to the ViewModel is killed (e.g. Navigated away from)
• Running in iOS using Swift UI.

PseudoCode:

class LoginViewModel : ViewModel  {

    val observable = Firebase.auth.currentUser.toObservable(coroutineScope) // Mpp implementation of Observable that lives during VM lifecycle

    init {
        coroutineScope.launch {
            Firebase.auth.authStateChanged.collect { state -> if (state == null) // navigate to different Screen }
        }
    }

}

Launching this crashes the app, because when logged out on init it immediately navigates away, closing the coroutineScope before the observable has its initial value. Offer is then fired despite the coroutine being cancelled, because the listener is always called when assigned. Its an edgecase of a concurrency issue, but it can and will happen. Jut not offering after the scope is cancelled solves the issue

[nbransby]

We use this code in production and its never happened I suspect it's a iOS only thing. Coroutines on ios are being majorly overhauled so maybe this problem will go away at some point.

Are you able to write a test to reproduce the issue? Maybe we just implement this workaround for iOS only for now?

Another thing I have noticed though is that we should probably be using sendBlocking instead of offer in the callbackFlows - this is unrelated to this issue but this would be a good time to make the switch.

[Daeda88]

I managed to reproduce it in Android as well:

class AuthViewModel(context: Context) : ViewModel() {

    val superVisorJob = SupervisorJob()

    val job = CoroutineScope(Dispatchers.Main + superVisorJob).launch { Firebase.auth.authStateChanged.first() }

    init {
        Firebase.initialize(context)

        viewModelScope.launch {
            Firebase.auth.authStateChanged.collect { user ->
                superVisorJob.cancelChildren()
            }
        }
    }

    fun start() {

    }

}

Crashes with

2020-10-20 22:44:48.221 20872-20872/? E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.daeda.sample, PID: 20872
    kotlinx.coroutines.JobCancellationException: StandaloneCoroutine was cancelled; job=StandaloneCoroutine{Cancelled}@9186938

[nbransby]

Do you have a full stack trace for that exception?

[Daeda88]

Nope, thats all I'm getting, most likely because it crashes within the coroutine. I found the source by commenting out the code until I found the source at offer. I also found this thread talking about similar issues: Kotlin/kotlinx.coroutines#1762

in addition, I would suggest it makes perfect sense this crashes:

actual val authStateChanged get() = callbackFlow {
        val listener = AuthStateListener { auth -> offer(auth.currentUser?.let { FirebaseUser(it) }) }
        android.addAuthStateListener(listener)
        awaitClose { android.removeAuthStateListener(listener) }
    }

Here, addAuthStateListener is not a suspend function. It just passes a listener object, so its callback can be called even after cancelling the coroutine. This is exactly what happens: if the coroutine is cancelled while the auth state changes, offer will be called, despite offer no longer being valid due to the cancelled coroutine. It a concurrency issue that is rare, except if you cancel exactly as you add the listener, since this results in an immediate callback.

So I understand why you dont run into it on production: unless you hit the edgecase it wont be noticable. But when you do, it breaks.

In any case, I dont see the harm in this change, since even if offer would just ignore the value when the coroutine is cancelled, this check just causes the same behaviour.

[nbransby]

Interesting, following the link you posted I got to Kotlin/kotlinx.coroutines#974

Looks like they will address this in the future but that thread mentioned it could still happen even using isClosedForSend.

Maybe we should surround them all with try catch?

[Daeda88]

used the try catch approach instead.

One thing I am slightly unsure about is whether the listeners are properly unsubscribed in these cases, but at least this will prevent a crash