copper-leaf / ballast

Opinionated Application State Management framework for Kotlin Multiplatform
https://copper-leaf.github.io/ballast/
BSD 3-Clause "New" or "Revised" License
144 stars 11 forks source link

Compose's onValueChange sometimes results in IllegalStateException: VM is cleared! #25

Closed asapha closed 2 years ago

asapha commented 2 years ago

Hi,

I'm seeing a few crashes from users exiting a specific screen. As the app uses Compose's NavHost, the culprit VM is scoped to a NavBackStackEntry.

I can't reproduce the crash on my devices but here's what I could gather:

To investigate, I've overridden my ViewModel's onCleared

class MyVM : AndroidViewModel<Inputs, Events, State>(
   [...]
    override fun onCleared() {
        Logs.i("onCleared")
        super.onCleared()
    }

And added logs to onValueChange

    onValueChange = {
        Logs.i("onValueChange")
        currentName = it
        vm.trySend(NameEditorContract.Inputs.ChangeName(it.text))
    }

Then, with a Pixel 4A 5G: It's possible to have onValueChange displayed after onCleared, but only if the cursor is not placed at the end of the OutlinedTextField. (Note that it doesn't crash for me as BallastViewModelImpl.onCleared hasn't been called yet)

But on a LGV30, onValueChange is never displayed after onCleared

Not sure if ballast is too strict or if compose doesn't respect some sort of contract here, what do you think?

Stacktrace:

Fatal Exception: java.lang.IllegalStateException: VM is cleared!
       at com.copperleaf.ballast.internal.BallastViewModelImpl.checkValidState(BallastViewModelImpl.kt:198)
       at com.copperleaf.ballast.internal.BallastViewModelImpl.trySend-JP2dKIU(BallastViewModelImpl.kt:102)
       at com.copperleaf.ballast.core.AndroidViewModel.trySend-JP2dKIU(AndroidViewModel.kt:2)
       at com.azefsw.audioconnect.settings.ui.name.NameEditorViewKt$NameEditorView$2$1$1.invoke(NameEditorView.kt:59)
       at com.azefsw.audioconnect.settings.ui.name.NameEditorViewKt$NameEditorView$2$1$1.invoke(NameEditorView.kt:57)
       at androidx.compose.foundation.text.BasicTextFieldKt$BasicTextField$7$1.invoke(BasicTextField.kt:279)
       at androidx.compose.foundation.text.BasicTextFieldKt$BasicTextField$7$1.invoke(BasicTextField.kt:277)
       at androidx.compose.foundation.text.TextFieldState$onValueChange$1.invoke(CoreTextField.kt:762)
       at androidx.compose.foundation.text.TextFieldState$onValueChange$1.invoke(CoreTextField.kt:757)
       at androidx.compose.foundation.text.TextFieldDelegate$Companion.onBlur$foundation_release(TextFieldDelegate.java:242)
       at androidx.compose.foundation.text.CoreTextFieldKt.onBlur(CoreTextField.kt:840)
       at androidx.compose.foundation.text.CoreTextFieldKt.access$onBlur(CoreTextFieldKt.java:1)
       at androidx.compose.foundation.text.CoreTextFieldKt$CoreTextField$2$invoke$$inlined$onDispose$1.dispose(Effects.kt:485)
       at androidx.compose.runtime.DisposableEffectImpl.c(Effects.kt:4)
       at androidx.compose.runtime.CompositionImpl$RememberEventDispatcher.dispatchRememberObservers(Composition.kt:995)
       at androidx.compose.runtime.CompositionImpl.applyChangesInLocked(Composition.kt:774)
       at androidx.compose.runtime.CompositionImpl.applyChanges(Composition.kt:794)
       at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:526)
       at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:454)
       at androidx.compose.ui.platform.AndroidUiFrameClock$withFrameNanos$2$callback$1.doFrame(AndroidUiFrameClock.android.kt:8)
       at androidx.compose.ui.platform.AndroidUiDispatcher.performFrameDispatch(AndroidUiDispatcher.java:109)
       at androidx.compose.ui.platform.AndroidUiDispatcher.access$performFrameDispatch(AndroidUiDispatcher.java:41)
       at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.doFrame(AndroidUiDispatcher.android.kt:69)
       at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1002)
       at android.view.Choreographer.doCallbacks(Choreographer.java:816)
       at android.view.Choreographer.doFrame(Choreographer.java:748)
       at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:990)
       at android.os.Handler.handleCallback(Handler.java:873)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at android.os.Looper.loop(Looper.java:193)
       at android.app.ActivityThread.main(ActivityThread.java:6762)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

ballast version 1.1.0 compose version: 1.2.0-beta02

cjbrooks12 commented 2 years ago

Hmm, that's super weird. The checks for the VM being cleared happen when the VM's CoroutineScope is cancelled (which is when the Androidx VM gets onCleared()). I'm wondering if something with the navigation lib transitions would be the main culprit (leave a screen > VM gets cleared > during transition the TextField loses focus and attempts to call onValueChange).

Regardless, it sounds like it would be better to make the trySend method less strict, than trying to fight the navigation lib. If the VM is cleared, return a failed ChannelResult rather than crashing. The vm.send() should probably remain strict, but vm.trySend() could be relaxed.

Could you open a PR to remove the check? Should be able to just remove the check from here

cjbrooks12 commented 2 years ago

This fix has been released and is available in version 1.2.1. Thanks again for the PR to fix it!