clj-commons / citrus

State management library for Rum
Eclipse Public License 1.0
274 stars 21 forks source link

Behaviour When Mixing Dispatch and Dispatch Sync #35

Open mbutlerw opened 6 years ago

mbutlerw commented 6 years ago

Hi, I brought this up on the slack, but it got lost due to no history.

There is some surprising behaviour when mixing dispatch and dispatch sync.

;; sync inside async gets lost
(defmulti controller (fn [event] event))

(defmethod controller :one []
  (println "one")
  {:state {1 true}
   :effect nil})

(defmethod controller :two [_ _ state]
  (println "two:" state)
  {:state (assoc state 2 true)
   :effect2 nil})

(defmethod controller :three [_ _ state]
  (println "three:" state)
  {:state (assoc state 3 true)
   :effect3 nil})

(defmethod controller :four [_ _ state]
  (println "four:" state)
  {:state (assoc state 4 true)})

(defn effect [r _ _]
  (citrus/dispatch-sync! r :controller :two))

(defn effect2 [r _ _]
  (citrus/dispatch-sync! r :controller :three))

(defn effect3 [r _ _]
  (citrus/dispatch-sync! r :controller :four))

(defonce recon
  (citrus/reconciler {:state (atom {})
                      :controllers {:controller controller}
                      :effect-handlers {:effect effect
                                        :effect2 effect2
                                        :effect3 effect3}}))

(citrus/dispatch! recon :controller :one)

The output of this is

one
two: nil
three: {2 true}
four: {2 true, 3 true}

and then if you print the reconciler once its all run

#object [citrus.reconciler.Reconciler {:val {:controller {1 true}}}]

The apparent behaviour is that first the update from the dispatch! isn't reflected in the subsequent dispatch-sync! method calls. However if you then check the state of the reconciler its only the result of the dispatch! that is reflected.

If you change effect to dispatch! as well, like so

(defn effect [r _ _]
  (citrus/dispatch! r :controller :two))

you get the following output

one
two: {1 true}
three: {1 true}
four: {1 true, 3 true}
#object [citrus.reconciler.Reconciler {:val {:controller {1 true, 2 true}}}]

So which indicates the "bug" is when you switch between dispatch! and dispatch-sync!

If you flip the problem and start from a dispatch-sync! and go to dispatch! it appears to work as expected.

(citrus/dispatch-sync! recon :controller :one)

(defn effect [r _ _]
  (citrus/dispatch! r :controller :two))

(defn effect2 [r _ _]
  (citrus/dispatch! r :controller :three))

(defn effect3 [r _ _]
  (citrus/dispatch! r :controller :four))

Output:

one
two: {1 true}
three: {1 true, 2 true}
four: {1 true, 2 true, 3 true}
#object [citrus.reconciler.Reconciler {:val {:controller {1 true, 2 true, 3 true, 4 true}}}]

Wanted to raise this, even if the answer is simply a warning about mixing dispatch and dispatch-sync.

roman01la commented 6 years ago

Thanks for reporting this. I’m on vacation now, will take a look later.

DjebbZ commented 6 years ago

I've been bitten by the same root cause of this problem just today.

For dispatch!, the side effects are always executed before changing the state. Here's the code for latest Citrus 3.2. For dispatch-sync!, the execution order between the :state effect and other side effects depends on the order of traversal of Citrus's custom doseq macro. See the code.

Makes me think that there's a problem of predictability and/or coherence, or at least a documentation problem. If code needs to be changed, I don't know how to make it non-breaking.

martinklepsch commented 4 years ago

I think with the the recent :citrus/handler refactorings (#50 & #59) the behavior should be more consistent:

https://github.com/clj-commons/citrus/blob/8ed6d0ecb0887d0f0807b40569cc9e8ab4b67c30/src/citrus/reconciler.cljs#L104-L114

Specifically :state will always be executed last:

https://github.com/clj-commons/citrus/blob/8ed6d0ecb0887d0f0807b40569cc9e8ab4b67c30/src/citrus/reconciler.cljs#L46-L54

I guess this is a breaking change in some ways and we will document this in the release notes.