Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handler State Become Stale for Actions That Produce Long Running Flows #29

Closed
solcott opened this issue Mar 11, 2021 · 1 comment
Closed

Comments

@solcott
Copy link
Contributor

solcott commented Mar 11, 2021

There are a couple ways I can see to fix this, neither of which is backwards compatible.

  1. Pass StateFlow<State> as parameter instead of State in Handler
typealias Handler<State, Action, Mutation> = (StateFlow<State>, Action) -> Flow<Mutation>

//Flow<Action>.collectIntoHandler changes to
handler.invoke(state, action, LoggingEffectSender(logger))

//Inside handler functions the current state can be accessed using
{ state, action ->
  state.value
}
  1. Pass function () -> State as parameter instead of State in handler
typealias Handler<State, Action, Mutation> = (() -> State, Action) -> Flow<Mutation>

//Flow<Action>.collectIntoHandler changes to
handler.invoke({state.value}, action, LoggingEffectSender(logger))

//Inside handler functions the current state can be accessed using
{ state, action ->
   state()
}

Example: Notice that MyState(count=1) and MyState(count=2) are printed when it should print all the way up to count=6

data class MyState(val count: Int = 1)
  sealed class MyAction {
    object Start : MyAction()
  }

  sealed class MyMutation {
    data class IncrementCount(val newCount: Int) : MyMutation()
  }

  @Test
  fun testStaleState() {
    runBlocking {
      val handler: Handler<MyState, MyAction, MyMutation> = { state, action ->
        when (action) {
          MyAction.Start -> flow {
            emit(MyMutation.IncrementCount(state.count + 1))
            kotlinx.coroutines.delay(100)
            emit(MyMutation.IncrementCount(state.count + 1))
            kotlinx.coroutines.delay(100)
            emit(MyMutation.IncrementCount(state.count + 1))
            kotlinx.coroutines.delay(100)
            emit(MyMutation.IncrementCount(state.count + 1))
            kotlinx.coroutines.delay(100)
            emit(MyMutation.IncrementCount(state.count + 1))
          }
        }
      }
      val reducer: Reducer<MyState, MyMutation> = { state, mutation ->
        when(mutation){
          is MyMutation.IncrementCount -> state.copy(count =  mutation.newCount)
        }
      }
      MVFlow(MyState(), handler, reducer, this).takeView(this, object : MVFlow.View<MyState, MyAction>{
        override fun actions(): Flow<MyAction> {
          return flowOf(MyAction.Start)
        }

        override fun render(state: MyState) {
          println(state)
        }
      })
    }
  }
@pedroql
Copy link
Owner

pedroql commented Mar 13, 2021

This is covered in the documentation:

Note: although the handler receives the current state when an action is performed, if the handler performs a slow operation, the handler will no longer know the most recent state. This is why the handler can’t mutate the state directly.

From https://pedroql.github.io/mvflow/#reducer

The reason we can't have the handler receiving the current state is because there's no way to guarantee that this is and remains the most up to date state while the handler does something with it because there could be multiple coroutines accessing and mutating the state.

Instead, MVFlow ensures that calls to the reducer don't interleave so it's the only safe place to update the state.

We do have unit tests for the scenarios you highlight here.

--

You need to update your code sample and use the mutations in a different way. In your mutations, you pass the desired final value. Instead, you should pass what you want to do to the state. So it goes from emit(MyMutation.IncrementCount(state.count + 1)) to emit(MyMutation.IncrementCount(1)). And your reducer (with a renamed property) becomes state.copy(count = state.count+ mutation.changeValue).

This is what we mean when we say

Keep also in mind that Mutations should indicate how to change the state, but should not rely on/assume what the current state is (as of when the action was emitted).

MVFlow.kt#L28

--

Let me know if you have ideas to improve the documentation so other people don't miss this important detail!

@pedroql pedroql closed this as completed Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants