badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
827 stars 64 forks source link

Job does not have time to cancel and calls render after a view is destroyed #127

Closed Skeptick closed 4 years ago

Skeptick commented 4 years ago

Hello! In my application, I store a navigation action in a state, and also use the NavController::addOnDestinationChangedListener to tell the Store that the navigation action has been completed and can be nullified. Thus, Store generates a new state at the very edge before destroying the view. Today I ran into the fact that 1 time out of 10, the state arrives in the render method too late, and the application crashes due to the fact that the view has already nullified. Changing BinderLifecycleMode from START_STOP to RESUME_PAUSE had no effect. I assumed that a coroutine does not have time to cancel Job and added yield() here, before assertOnMainThread(), and it seems to help. I was unable to reproduce the problem again. It will be great if you do it in the library :)

arkivanov commented 4 years ago

Hello @Skeptick, would you mind to provide a reproducer?

Skeptick commented 4 years ago

I'll try to do it on the weekend if I have time.

arkivanov commented 4 years ago

Thanks, this would really helpful!

Skeptick commented 4 years ago

Due to the fact that the application and views are very simple, the problem is reproduced much less often, but sooner or later it is reproduced :)

sample.zip

arkivanov commented 4 years ago

Thanks, I will check!

Skeptick commented 4 years ago

Also, for reproduce, you can run a similar coroutine when creating a Store:

override suspend fun executeAction(action: Action, getState: () -> State) {
    coroutineScope {
        launch {
            while (isActive) {
                delay(1)
                dispatch(Result.SomeResult)
            }
        }
    }
}

That is, my case is not particularly rare. This can be achieved simply by having a Job updating the view state in the background. I checked, yield() or if (coroutineContext.isActive) {... or if (job?.isActive == true) {... in binder also solved this problem.

arkivanov commented 4 years ago

@Skeptick Thanks! Could you please elaborate what you mean by having a Job updating the view state in the background. View updates should be performed always on the main thread.

Skeptick commented 4 years ago

Oh yes, this is probably a bad description. I didn't mean a background thread, but something like "background work".

arkivanov commented 4 years ago

@Skeptick Thanks for the reproducer, this was really helpful.

So, the binder uses the Main dispatcher by default, which posts events asynchronously. Actual view update happens when the Lifecycle is already destroyed and the corresponding Job is cancelled. This really looks like a bug in coroutines machinery. The coroutine is resumed from the suspention point and is being executed even if it is already cancelled. I would consider to file an issue for this case with a simplier reproducer.

I will add one of the checks suggested by you, thanks!

As a workaround you can use Dispatchers.Main.immediate in the following way:

        bind(viewLifecycle, BinderLifecycleMode.CREATE_DESTROY, Dispatchers.Main.immediate) { view.events bindTo store }
        bind(viewLifecycle, BinderLifecycleMode.START_STOP, Dispatchers.Main.immediate) { store.states bindTo view }

In this case view updates will be performed in the same callstack.

arkivanov commented 4 years ago

I will also consider using Dispatchers.Main.immediate by default, if possible.

Skeptick commented 4 years ago

Oh, thank you!