Race condition issue with <-deref

[orestis]

The more I use hx, the more I like it. Thanks so much!

I ran across a baffling race condition when using <-deref. I haven't created a repro yet, but as far as I can tell, it goes like this:

1. Setup some top-level atom
2. Kick off a network request that will eventually swap! into that atom.
3. Render a component that uses <-deref on that atom.
4. Component is rendered using the current state
5. Network request comes back, updates the atom <--- MISSED UPDATE
6. useEffect is called to add a watch to the atom

If steps 5 and 6 are swapped (because of a slower network etc) then everything works fine.

I worked around this issue by adding an extra call to setState after the watch is added. This might lead to an unnecessary render but I'm not sure.

It seems that useEffect is called asynchronously after rendering is done, and that is probably the source of the race condition.

[lilactown]

Wow! Great find. Your description makes some sense to me; useEffect definitely fires after rendering is done. I can see how the race condition occurs; that's a tricky one!

I can think of two potential fixes:

1. Use useLayoutEffect instead to fire synchronously after render

However, this might have negative effects; per the React docs:

Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint.

This means that when Concurrent React lands, we might be over-scheduling the state changes to re-render the component.

This actually begs the question how re-renders are being scheduled now while we're using useEffect. 🤔

2. Manually manage the subscription via refs

Something like:

(let [[v u] (<-state @a)
     atom-identity (<-ref nil)
     sub (<-ref (gensym "<-"))]
  (when (and (not= a @atom-identity)
    (when (not (nil? @atom-identity))
      (remove-watch a @sub))
    (add-watch a @sub (fn [_ _ _ v'] (u v'))))
  v)

This completely removes the act of subscribing from React's lifecycle. But once again I'm not sure how this interacts with React's scheduler.

I might reach out to the React team to see the best way to handle this case. I imagine this would come up anytime you're subscribing to some external event bus.

[lilactown]

Looks like there is already an issue in the React repo: facebook/react#14988

gaeron posted this:

As for fixing the original issue, the strategy is to read the mutable value in useEffect. If it changed since the value captured during render, setState.

Which seems like an easy fix.

[orestis]

Good catch! This is the fix that I also applied, just a single line that calls setState. It’s unconditional, but doing it conditionally should be straightforward (if a little bit unnecessary?)

…

Wow! Great find. Your description makes some sense to me; useEffect definitely fires after rendering is done. I can see how the race condition occurs; that's a tricky one! I can think of two potential fixes: 1. Use useLayoutEffect instead to fire synchronously after render However, this might have negative effects; per the React docs: Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint. This means that when Concurrent React lands, we might be over-scheduling the state changes to re-render the component. This actually begs the question how re-renders are being scheduled now while we're using useEffect. 🤔 2. Manually manage the subscription via refs Something like: (let [[v u] (<-state @A) atom-identity (<-ref nil) sub (<-ref (gensym "<-"))] (when (and (not= a @atom-identity) (when (not (nil? @atom-identity)) (remove-watch a @sub)) (add-watch a @sub (fn [_ _ _ v'] (u v')))) v) This completely removes the act of subscribing from React's lifecycle. But once again I'm not sure how this interacts with React's scheduler. I might reach out to the React team to see the best way to handle this case. I imagine this would come up anytime you're subscribing to some external event bus. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[lilactown]

Would you be able to verify that 6d3b7cb fixes the issue?

[orestis]

I’ll have to test it on Monday, but I applied pretty much the same code (sans comparison) in a local version and that did the trick. The comparison should be ok, since there can’t be a second thread mutating the atom behind the scenes.

Thanks!

[lilactown]

Cool. Closing this for now. Re-open if you still run into issues.