Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

Race condition for side effects #128

Closed
LocusLabsGit opened this issue Jan 30, 2020 · 9 comments
Closed

Race condition for side effects #128

LocusLabsGit opened this issue Jan 30, 2020 · 9 comments
Labels
bug Something isn't working question Further information is requested

Comments

@LocusLabsGit
Copy link

My understanding was that side effects were supposed to be evaluated after each reduction but what I'm seeing are race conditions instead.

Basically what I'm trying to do is automatically show a window ("Window1") after another window ("Window2") is hidden.

Background
Window1 should not always be shown after Window2 is closed. It depends on whether Window1 was originally open before the user tapped a button on Window1 that opened Window2.

User Scenario A:

  1. Open Window1
  2. Tap button on Window1 to open Window2
  3. Close Window2
  4. Reopen Window1

User Scenario B:

  1. Open Window2
  2. Close Window2
  3. (Window1 should not get opened)

I have the following actions:

ShowWindow1
HideWindow1
ShowWindow2 (parameter: returnToWindow1: true/false)
HideWindow2

When the user taps the button on Window1, it dispatches ShowWindow2 with returnToWindow1=true. When the user taps a button on the main screen to open Window2, it dispatches ShowWindow2 with returnToWindow1=false.

Moreover, I have a "render" function which observes changes to the state. Render takes care of opening and closing windows. To record that it's done rendering, it dispatches these actions:

Window1Shown
Window1Hidden
Window2Shown
Window2Hidden

Render and reduce work together with booleans. For example when HideWindow2 gets reduced, it changes a boolean in the state, isWindow2Hiding, to true. The render function looks for isWindow2Hiding and if it's true, it closes Window2. Then it dispatches Window2Hidden which sets isWindow2Hiding to false.

Problem
After my reducer handles HideWindow2, I expect my side effect to see state changed by HideWindow2. But instead the side effect sees the state changed not only by HideWindow2 but also Window2Hidden!

One correct behavior is the side effect is called twice (once for HideWindow2 and once for Window2Hidden). HOWEVER, the state the side effect sees is that of Window2Hidden. Consequently, the variable returnToWindow1 has already been changed from true to false so the the side effect doesn't know it should dispatch ShowWindow1.

Workaround
Since this is a race condition problem, I found I could work around it by introducing a delay in my render function. When render sees that isWindow2Hiding==true, it closes the window and waits 10 milliseconds before dispatching Window2Hidden. This is just enough time for the right order of operations:

  1. Dispatch HideWindow2
  2. Reduce HideWindow2 setting isWindow2Hiding=true
  3. Renders sees isWindow2Hiding==true so closes the window and waits 10 milliseconds...
  4. Meanwhile side effect sees returnToWindow1==true so it dispatches ShowWindow1
  5. ...renderer wakes up and dispatches Window2Hidden
  6. Reduce Window2Hidden setting isWindow2Hiding=false

Why doesn't the side effect get called after the first action is reduced? Why does it reduce two actions before evaluating the side effect?

@Tapchicoma
Copy link
Contributor

Is it possible to provide some sample project to better understand what is your problem and how have you defined state machine?

@Tapchicoma Tapchicoma added bug Something isn't working question Further information is requested labels Feb 6, 2020
@LocusLabsGit
Copy link
Author

Because I've worked around the problem, I can't invest a lot of time in creating a sample project. I wrote the feedback above in case there is in an opportunity to improve CoRedux for others who have a similar use case. If I do find the time to publish a sample project, I will post a link to it.

@LocusLabsGit
Copy link
Author

@Tapchicoma I am still seeing this race condition so I'm providing https://github.com/mosofsky/CoReduxSideEffectRaceCondition as a sample project to help you better understand the problem. I eagerly await your feedback.

@LocusLabsGit
Copy link
Author

I seem to have found a solution by implementing SideEffect instead of instantiating SimpleSideEffect.

SimpleSideEffect and CancellableSideEffect seem to be more appropriate when you want to cancel the side effect if a new action of the same type comes in too quickly. I did not want that behavior for this situation.

I published the solution as an update to the sample project (https://github.com/mosofsky/CoReduxSideEffectRaceCondition)

@Tapchicoma
Copy link
Contributor

Thank you for repro project - I will check how you've solved this race-condition 👍

@Tapchicoma
Copy link
Contributor

I think the reason you had this was:

  • SimpleSideEffect handler is suspend higher order function that used MyViewModel CoroutineScope to run
  • scope for MyViewModel is never disposed, though it should be
  • in your fix you used viewModelScope that is autodisposed with related ViewModel

@LocusLabsGit
Copy link
Author

LocusLabsGit commented Apr 23, 2020

Thank you @Tapchicoma . I thought the problem was because SimpleSideEffect contains the following logic for canceling prior responses if they're still active:

                job?.run {
                    if (isActive) {
                        logger.logSideEffectEvent {
                            LogEvent.SideEffectEvent.Custom(
                                name,
                                "Cancelling previous job on new $action action"
                            )
                        }
                    }
                    cancel()
                }

source: https://github.com/freeletics/CoRedux/blob/master/library/core/src/main/kotlin/com/freeletics/coredux/SideEffectHelpers.kt

@Tapchicoma
Copy link
Contributor

Yeah, idea behind SimpleSideEffect is that it is kind of "one-time" and works like RxJava switchMap { } operator cancelling previous running operations on a new input

@mosofsky
Copy link

Thanks again for all your help @Tapchicoma . CoRedux is working really well for my project now that I've overcome the challenge of handling asynchronous UI events. CoRedux has scaled to 91 actions and 80 state variables. Great work guys!

To demonstrate how I've managed to handle asynchronous UI code, I created another sample project at https://github.com/mosofsky/how-to-coredux . This second sample project builds on the first by showing how CoRedux side effects can facilitate asynchronous UI testing via Espresso's idling resource counter.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working question Further information is requested
Development

No branches or pull requests

3 participants