Bug: is the current `useSyncExternalStore` batching & flushing behaviour intended?

[phryneas]

React version: 18

Link to code example:

CodeSandbox

The expected behavior

Let's assume we have a increment function that first increments a local value, then a uSES value and then another local value like this:

function increment() {
    setState1((x) => x + 1);
    setUsesState((x) => x + 1);
    setState2((x) => x + 1);
  }

Now, there would be two ways this could behave that would be "intuitive for me":

1. everything is batched: [state1, usesState, state2] goes from [0,0,0] to [1,1,1]
2. it batches until the "sync update" flushes the current batch: [state1, usesState, state2] goes from [0,0,0] to [1,1,0] to [1,1,1]

The current behavior

Now, actual behaviour is different in React 18, depending on the "mode" React is currently in.

1. in an event handler, everything is batched [0,0,0] to [1,1,1] - no problem here
2. outside an event handler, the uSES setter is flushed first, then the local state changes are batched. [0,0,0] becomes [0,1,0] becomes [1,1,1] - this is very unintuitive for me.
3. even inside a manual call wrapped in unstable_batchedUpdates, we go [0,0,0] -> [0,1,0] -> [1,1,1]

Point 3 means that there is actually no way to even manually batch an update by uSES - but looking at point 1, React sometimes does so internally.

It seems that even in the non-batched situations, React does some batching: Calling setUsesState twice before calling setState2 will not lead to a [0,0,0] -> [0,1,0] -> [0,2,0] -> [1,2,1] situation, but to [0,0,0] -> [0,2,0] -> [1,2,1]

Up until now we had assumed that uSES would always behave like in 1., and we were only made aware of this by bug reports on react-redux.

Is this intended behaviour or a bug?

There might be some high priority update thing with a transition that I am missing here though - but either way this feels very breaking from older behaviour to me - and yes, I know that batchedUpdates has the unstable prefix ;)

[dai-shi]

fwiw, startTransition seems to make the behavior more consistent.
https://codesandbox.io/s/uses-behaviour-react18-forked-pmq3o1
so, the onClick handler would be an exception with higher priority.

(btw, logMethod is a nice hack.)

[Andarist]

Related issue: #24831

[phryneas]

fwiw, startTransition seems to make the behavior more consistent. codesandbox.io/s/uses-behaviour-react18-forked-pmq3o1 so, the onClick handler would be an exception with higher priority.

Even with the onClick={() => startTransition(increment)}, I am seeing [0,0,0] -> [0,1,0] -> [1,1,1] - or am I missing something?

[dai-shi]

I meant only the in-consistent behavior is bare onClick. I didn't mean which is correct or expected.

[sebmarkbage]

It's because useSyncExternalStore always has to be flushed at the highest priority where as other updates can be delayed. It can't be delayed because of the "Sync" part which is a compromise to using this API since it trades preserving internal consistency with the external store by compromising the ability to delay it. So it can't be batched but it also can't be time sliced and deprioritized.

You can regain consistency by flushing other updates together with this one by using flushSync(...) but then you're compromising by making all other updates not delayed neither.

If it was batched, which is basically what you'd get by manually subscribing without using it, you lose other types of consistency due to using a mutable backing store.

[sebmarkbage]

In React <=17 the default for setState was sync - you could opt-in to batched with batchedUpdates.

In React 18 the default is batched - you can opt-out to sync with flushSync.

useSyncExternalStore is always sync to preserve the consistency with other mutable data.

[Andarist]

Hm, while priority-based updates are probably a powerful feature - it's really hard to grasp how this works in edge cases like this. It feels like there should be a way to somehow "join" those updates without resorting to flushSync. In this case, it would be quite convenient to change the priority of the update with the default priority if there is already an ongoing "sync" update. Correct me if I'm wrong but the uSES update is still not exactly synchronous - all updates coming from that store are batched together and more often than not it is desirable to flush other updates with those. Part of the problem is that people usually won't even interact with uSES directly but rather through a library. In those situations, it's even harder to notice that one might deal with such a discrepancy in flushed updates.

[sebmarkbage]

The interesting thing about these things is whether it's observable behavior or a perf consideration. If the useEffects observing the rendering or painting by the browser, while still being implemented idiomatically, observe this difference in a negative way. From what I can tell by the reports, these are just a perf concern rather than a semantic concern.

The perf concern might be valid though.

I don't see it being possible to delay useSyncExternalStore updates beyond an event loop. It opens up a lot more complexity which this feature isn't really meant to address.

However, grouping sync, discrete and default updates into a single lane and flushing them at the highest priority available opportunity is consistent with the model. In other words, flushing useSyncExternalStore and other updates in the same event loop together in the next tick would be valid.

The downside of that approach is that it means that you might be relying on batching of many small updates. That's the idea that you shouldn't have to think about throttling and stuff. That should be automatic in the model. But just adding a call to something that uses sync external store might deopt the whole app and causing those to suddenly become a perf issue. That might not be a huge issue though.

In particular setState in useEffect is tricky because you don't want to cause these multiple synchronous passes, but setState in useEffect is so trick.

Ofc, on the flip side, rendering twice might also be an issue.

It would certainly simplify the model if it was a single lane.

[Andarist]

The interesting thing about these things is whether it's observable behavior or a perf consideration.

The first minimal~ repro case in this thread showcases this problem through an observable behavior (not a perf issue)

However, grouping sync, discrete and default updates into a single lane and flushing them at the highest priority available opportunity is consistent with the model. In other words, flushing useSyncExternalStore and other updates in the same event loop together in the next tick would be valid.

Yeah, I was asking about this - not the other way around. It's good to know that this wouldn't break the model.

[rickhanlonii]

all updates coming from that store are batched together and more often than not it is desirable to flush other updates with those

What types of updates are you thinking of? They would need to be updates that are not already marked with a priority, right?

[Andarist]

A user has reported to us that something like this (conceptually) wasn't flushed together:

// executed asynchronously somehow, not called from the event handler
function increment() {
    setState1((x) => x + 1);
    setUsesState((x) => x + 1);
}

This led to the situation in which useEffect in the parent couldn't "see" the updated DOM from the child because the child was dependent on that "default" update (the setState1 here) while the parent was depending on the "sync" update from the sync store (the setUsesState here).

So if I interpret the question correctly - then yes, that other update was not explicitly marked with any priority.

[phryneas]

Even with only one component, this could lead to useEffect side effects for combinations of local state and global state that should - from a user's perspective - be impossible.

[rickhanlonii]

I mean, where was increment called?

What would you expect if it was called as:

startTransition(() => {
  increment();
})

[Andarist]

One of the user reports that we have received can be found here. In this case, there is a handleFetch function that calls regular setState but it is itself called by XState when a promise resolves. XState also changes its state when that promise gets resolved and that state is using uSES.

What would you expect if it was called as:

I'd expect both updates to be flushed together at some point. I wouldn't expect the uSES update to be flushed immediately, despite it being "sync". Since both updates are wrapped with startTransition I don't expect the uSES update to be reflected on the screen immediately.