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

Fix OM 862 - Race condition between schedule-render! and send cb's reconcile! that prevents some future reconciling #863

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bnoguchi
Copy link
Contributor

@bnoguchi bnoguchi commented Apr 5, 2017

Fixes GH-862.

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.

@bnoguchi bnoguchi changed the title Fix race condition between schedule-render! and send cb's reconcile! that preventing future reconciling Fix OM 862 - Race condition between schedule-render! and send cb's reconcile! that preventing future reconciling Apr 5, 2017
@bnoguchi bnoguchi changed the title Fix OM 862 - Race condition between schedule-render! and send cb's reconcile! that preventing future reconciling Fix OM 862 - Race condition between schedule-render! and send cb's reconcile! that prevents some future reconciling Apr 5, 2017
Otherwise, this doesn't work when merged with om-860 because the devcards for
om-860 also exposes a race condition with the overriding om/*raf* we define in
this branch.
@swannodette
Copy link
Member

@bnoguchi thanks will review

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

Successfully merging this pull request may close these issues.

2 participants