omcljs / om

ClojureScript interface to Facebook's React
6.66k stars 363 forks source link

A race condition between schedule-render! and send cb's reconcile! that prevents some future reconciling #862

Open bnoguchi opened 7 years ago

bnoguchi commented 7 years ago

I discovered a race condition that prevents future reconciling from occurring. I've submitted a PR GH-863 with a devcards test and a fix.

Explanation

Right now, this block of code can result in a race condition that can stop future reconciling from occurring. Block of code inlined next for convenience:

            ([resp query remote]
             (when-not (nil? remote)
               (p/queue! this (keys resp) remote))
             (merge! this resp query remote)
             (p/reconcile! this remote))))))))

In particular, (merge! ...) will cause a state change that can trigger a (schedule-render! ...) and therefore eventually a reconcile. In cases where the requestAnimationFrame immediately runs the scheduled reconcile, "eventually" becomes "immediately." During this immediate reconcile, the following line will set :queued to false:

      (swap! state update-in [:queued] not)

In this scenario, the last(p/reconcile! this remote) in the code block above will run right after the reconcile triggered by requestAnimationFrame. This reconcile will also call the following line:

      (swap! state update-in [:queued] not)

which will set :queued back to true, without adding enqueueing anything to :queue-remote or :queue.

This puts our reconciler into a state in which subsequent calls to schedule-render! will think that there is already a render scheduled and won't queue a render and reconcile.

   (defn schedule-render! [reconciler]
     (when (p/schedule-render! reconciler)
       (queue-render! #(p/reconcile! reconciler))))

where p/schedule-render! is defined as follows:

  (schedule-render! [_]
    (if-not (:queued @state)
      (do
        (swap! state assoc :queued true)
        true)
      false))

This means that any reconciles that would result from a change in our reconciler :state won't trigger a reconcile. See here:

        (add-watch (:state config) (or target guid)
          (fn [_ _ _ _]
            (swap! state update-in [:t] inc)
            #?(:cljs
               (if-not (iquery? root-class)
                 (queue-render! parsef)
                 (schedule-render! this)))))

And the symptom will be observed state changes not re-rendering our ui.