use-subscription causes UI tearing in some random cases

[milankinen]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Referring to this twitter conversation, it seems that use-subscription can cause "UI tearing" in some random cases due to a (possible) race condition w.r.t. the combination of subscribe and getCurrentValue in internal usage.

Here is a minimalistic application demonstrating the behaviour: https://github.com/milankinen/use-subscription-tearing-demo

What is the expected behavior?

I'd expect counters in the example application to always render same number.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Tested React version: 16.9.0 (haven't tested with older ones)
OS: OSX 10.14.6
Hardware: MacBook Pro (Retina, 15-inch, Mid 2015) 2.8 GHz Intel Core i7 & 16GB RAM
Browser: Chrome Version 76.0.3809.100 (Official Build) (64-bit)

@bvaughn

[gaearon]

cc @bvaughn

[bvaughn]

This is an interesting case.

For me, the first mutation pair (that doesn't tear) proceeds like so:

1. Mutation happens
2. All useSubscription change callbacks fire and schedule work with React
3. React eagerly runs the state updater functions for all components
4. Async rendering starts, yielding every once and a while because components are intentionally expensive
5. Second mutation fires while rendering is still in progress (between frames).
6. Updates are scheduled with React, but updater functions aren't run until after the rendering finishes.

For the second mutation pair (the one that tears) the sequence is different in an important way:

1. Mutation happens
2. All useSubscription change callbacks fire and schedule work with React
3. State updater functions don't run
4. Async rendering starts
5. Individual state updater functions are run before each component is rendered
6. The second mutation fires between these renders
7. Subsequent state updater functions that run get the newer value.

I could "fix" the tearing in this case by reading the value in the outer change handler (outside of the state updater function, rather than inside of it) and I probably should make this change anyway. I'd like to better understand why this is happening first though.

I'm not sure yet why the different behavior occurs. It looks like React doesn't eagerly compute the state in the second case because expirationTime !== NoWork. I'm not sure if this is expected though, given that (at least in my test) I've flushed all pending work.

The good news is that I can catch this in a test. I had trouble at first, until I realized that the first interruption didn't cause the tear - but the second one did. (Test also requires two.)

[bvaughn]

cc @milankinen @dai-shi Have you retried the latest version of use-subscription (with the fix from #16623) to see if you still see tearing?

[dai-shi]

I tried with v1.1.0 and confirmed that tearing is fixed.

https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode/tree/d44142e853f324354633e6e4265b4ef2c9b3ddf8

Still check4 is failing. It might be rare edge cases.
It's a case a parent component renders during heavy render in children. Somehow it stops updating. Users can see it. Appreciated if you could take time to review it. (I tried to organize my test code since you previously mentioned some difficulties in reading it).

[bvaughn]

Hm. I don't have time to look now (preparing for a conf talk) but what you're describing sounds unexpected. If a parent renders, but it's children don't- React shouldn't commit anything to the DOM (until the children render).

Can you point me at that specific test case? (As a reminder for when the conf talk is done?)

[bvaughn]

@tsolman You subscribed yourself to this issue. You can unsubscribe from it by clicking the "Unsubscribe" button. No one can do that for you.

[milankinen]

Seems to fix my issue. Thank you! 👍

[dai-shi]

@bvaughn (I enjoyed the video of your talk at react conf 2019!)

Here's some pointers:

https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode/blob/d44142e853f324354633e6e4265b4ef2c9b3ddf8/__tests__/all_spec.js#L78-L92

https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode/blob/d44142e853f324354633e6e4265b4ef2c9b3ddf8/src/use-subscription/index.js#L45

I'm not sure if you can get the intention behind it (it's simple though.)
If not, I cold try reproducing it in codesandbox. So, please let me know.

[bvaughn]

Reducing the repro to a single failing case would probably be helpful, if it wouldn't be too much trouble :)

[dai-shi]

@bvaughn Here's the sandbox.

https://codesandbox.io/s/nice-tharp-6f6es

Strangely, I see tearing with it. (because it's DEV??)

Clicking "force update" while counting up tears and stops counting.

[dai-shi]

Strangely, I see tearing with it. (because it's DEV??)

This is no longer happening. Not sure, but react@experimental is updated?

Clicking "force update" while counting up tears and stops counting.

This is still happening.