useMutableSource: bugfix for new getSnapshot with mutation #18297
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
from
e07253b
to
bc57bac
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e07253b:
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 86da5ce:
|
Details of bundled changes.Comparing: a8f2165...86da5ce react-test-renderer
react-dom
react-art
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: a8f2165...86da5ce react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
from
bc57bac
to
7af270e
Compare
| // It's important to ensure that this update is included with that one. | ||
| // TODO This sucks; what's the right way of doing this? | ||
| runWithPriority(UserBlockingPriority, () => { | ||
| setSnapshot(maybeNewSnapshot); |
There was a problem hiding this comment.
As I mentioned in this comment, I think what I need to do here is schedule a state update with the same expiration time as the one that might be pending due to a mutation. (Either that or manually replace the pending update with this one, but that seemed a little shady too.)
Added this "fix" in the meanwhile, for discussion purposes. 😄
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
3 times, most recently
from
f5e7746
to
fde2923
Compare
|
I know you've been busy @acdlite, but friendly ping on this 😄 Dave was talking to me this afternoon about a bug he's seeing with Recoil and I'd like to at least sync this so we can see if the fix helped or not. |
| setSnapshot(() => { | ||
| throw error; | ||
| }); | ||
| latestSetSnapshot( |
There was a problem hiding this comment.
I think you need to do the same markRootExpiredAtTime thing during initial mount and when resubscribing, too, to avoid a momentary flicker.
There was a problem hiding this comment.
Hm...do we?
A change firing between render and subscription means that we missed an update, but we have other checks in place to avoid actually tearing between other components that read from the source.
There was a problem hiding this comment.
Put another way, the reason we added it where we did was because there's a potential tear- but in the subscribe-on-commit case, there's not a tear, just a potential missed update. Also at that point, we would have already shown the older value because we subscribe in a passive effect.
Unless I'm misunderstanding what you're suggesting?
There was a problem hiding this comment.
Follow up on the Dave /Recoil comment: Been stepping through that today, and it looks like the Recoil getter is also a setter, which is causing a loop. (In other words I think it's a Recoil problem, not a uMS problem.)
|
I think this test captures the mount/flicker case we talked about: it("blah", async () => {
const source = createSource("one");
const mutableSource = createMutableSource(source);
let committedA = null;
let committedB = null;
const onRender = () => {
if (committedB !== null) {
expect(committedA).toBe(committedB);
}
};
function ComponentA() {
const snapshot = useMutableSource(
mutableSource,
defaultGetSnapshot,
defaultSubscribe
);
Scheduler.unstable_yieldValue(`a:${snapshot}`);
React.useEffect(() => {
committedA = snapshot;
}, [snapshot]);
return <div>{`a:${snapshot}`}</div>;
}
function ComponentB() {
const snapshot = useMutableSource(
mutableSource,
defaultGetSnapshot,
defaultSubscribe
);
Scheduler.unstable_yieldValue(`b:${snapshot}`);
React.useEffect(() => {
committedB = snapshot;
}, [snapshot]);
return <div>{`b:${snapshot}`}</div>;
}
// Mount ComponentA with data version 1
act(() => {
ReactNoop.render(
<React.Profiler id="root" onRender={onRender}>
<ComponentA />
</React.Profiler>,
() => Scheduler.unstable_yieldValue("Sync effect")
);
});
expect(Scheduler).toHaveYielded(["a:one", "Sync effect"]);
expect(source.listenerCount).toBe(1);
// Mount ComponentB with version 1 (but don't commit it)
act(() => {
ReactNoop.render(
<React.Profiler id="root" onRender={onRender}>
<ComponentA />
<ComponentB />
</React.Profiler>,
() => Scheduler.unstable_yieldValue("Sync effect")
);
expect(Scheduler).toFlushAndYieldThrough(["a:one", "b:one", "Sync effect"]);
expect(source.listenerCount).toBe(1);
// Mutate -> schedule update for ComponentA
Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () => {
source.value = "two";
});
// Commit ComponentB -> notice the change and schedule an update for ComponentB
expect(Scheduler).toFlushAndYield(["a:two", "b:two"]);
expect(source.listenerCount).toBe(2);
});
}); |
|
I think the problem with what we talked about - only storing the lowest priority (highest expiration time) pending update - is that we are trying to use a single value for two things:
To be clear, I think the tests only happened to work before. I think this was a potential problem then too. For now I'll push the new flicker test, with the fix, but I'm not sure how to work around this second issue. |
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
from
fde2923
to
1383900
Compare
|
For the other types of pending work, we track the range: react/packages/react-reconciler/src/ReactFiberRoot.js Lines 66 to 73 in 58afba0 We can do the same thing here |
Yeah. That's what I ended up thinking too. I'll take a pass at it soon. FWIW, I did try doing a naive version of this yesterday (just tracking lowest and highest priority) and it didn't seem sufficient, but I'll take another pass. Got distracted by some other reviews. |
|
Gah. I had a small, stupid mistake in my first try at the separate expiration times, and it just took me 2 hours of staring at the code to spot it. |
|
Ok back to you @acdlite |
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
from
1383900
to
99a8fc7
Compare
| // We missed a mutation before committing. | ||
| // It's possible that other components using this source also have pending updates scheduled. | ||
| // In that case, we should ensure they all commit together. | ||
| markRootExpiredAtTime(root, getLastPendingExpirationTime(root)); |
There was a problem hiding this comment.
Need to mark this new setSnapshot call as a pending mutation, too, right before marking it as expired.
There was a problem hiding this comment.
This change makes sense conceptually, but it breaks the most recent test case I added.
In this new test I...
- Mount and flush component A with data version 1.
- Mount component B with data version 1 (but flush passive effects).
- Mutate the data (which schedules an update for already mounted component A.
- Flush passive effects (which also schedules an update for the newly mounted component B).
- Verify there was no tearing.
Before this change, both components A and B rendered together after we flushed passive effects.
After this change, component B renders and commits, then component A (and we tear in between). This is because- with the change in place, we're marking the root as expired with a higher priority (e.g. 1073741296 instead of 2/idle).
I think this indicates that we should be marking root as expired with the first expiration time rather than the last in this case? At least, doing that fixes this test (and also the other two failing tests that were tearing before). This also means that we aren't directly using last expiration time for anything at this point (although indicate it determines when we reset first expiration time in clearPendingUpdates).
…onger in use "first" time value
bvaughn
force-pushed
the
useMutableSource-bugfix
branch
from
7c9181f
to
db511f4
Compare
Ah. Assuming you just mean in this one place (and not everywhere) then I think tracking the "first" time may not be necessary. (Same thing as I said with my comment above, but with the labels swapped since I still can't make my brain think of the Going to push this change as a separate commit as well, to make it easier for you to review in isolation: db511f4 |
| ) { | ||
| if (didGetSnapshotChange) { |
There was a problem hiding this comment.
Can remove this nested check. Need to drop updates from new sources, too.
There was a problem hiding this comment.
Yeah, I wasn't sure about that... I'll remove it 😄
Thanks for the review!
There was a problem hiding this comment.
Hm, I think removing this check actually breaks some subtle assumptions about the effects and when we need to update the getSnapshot/setSnapshot refs. I think it would cause us to drop updates for mutations after a source changed... need to think about it a bit.
There was a problem hiding this comment.
I think it works because when source or subscribe changes, you're going to unsubscribe/resubscribe.
When getSnapshot changes, you don't resubscribe but you do track the latest value with a ref. Which I think of as an optimization for resubscribing; you're still "changing" the subscription in both cases. So they are the same thing except for what we do in the commit phase. (For example, I think all your tests would pass if you removed the effect and ref that tracks getSnapshot and instead added getSnapshot as a dependency to the resubscribe hook.)
There was a problem hiding this comment.
Ah, I see how to fix it. Will let me simplify things a bit too...
There was a problem hiding this comment.
Okay, here's what 86da5ce did:
Recreating the queue and dispatch if source or subscribe changed too (not just getSnapshot) meant that the we needed to update refs.setSnapshot in that case too. (Before that effect was only run when getSnapshot changed.)
So I added source or subscribe as dependencies to the first effect so it would update the dispatch ref. Since we're now also running this effect whenever we resubscribe, I was also able to consolidate the check for a changed snapshot between render and passive effects into just the first effect (rather than both).
There was a problem hiding this comment.
Weird. I didn't see any of your interleaved commits until just now, after I left mine. Heh. Didn't mean to seem like I was ignoring you.
For example, I think all your tests would pass if you removed the effect and ref that tracks getSnapshot and instead added getSnapshot as a dependency to the resubscribe hook.
This requires us to over-subscribe though, (since we can't share that effect without running the previous cleanup function and unsubscribing). Once we replace useEffect with our own custom pushed effect, we can optimize this though.
There was a problem hiding this comment.
To be clear, I'm trying to avoid re-subscribing unless actually necessary (not just convenient) because subscriptions can be expensive for some of useMutableSource's users. That was the whole reason to use a ref in the first place for get/set snapshot.
There was a problem hiding this comment.
Right, I can see why you did it this way. I think we might want to revisit the trade off in the future, though, since changing getSnapshot is expensive regardless because it leads to deopts. So it needs to be relatively stable. (I am kinda worried about this, since it's easy for users to mess up.) And if we assume it's relatively stable, then resubscribing might not be so bad.
There was a problem hiding this comment.
since changing getSnapshot is expensive regardless because it leads to deopts. So it needs to be relatively stable.
Yeah, absolutely. Maybe there's something we could do to detect when people are passing inline functions, so we could warn.
| // In that case, the subscription below will have cloesd over the previous function, | ||
| // so we use a ref to ensure that handleChange() always has the latest version. | ||
| // but this hook recreates the queue in certain cases to avoid updates from stale sources. | ||
| // handleChange() below needs to reference the dispatch function without re-subscribing, |
There was a problem hiding this comment.
I think if you go with this approach then you don't have to track getSnapshot or setSnapshot at all. Can use the closed over values, since getSnapshot is now a dep, and setSnapshot only changes when one of the other deps does.
There was a problem hiding this comment.
Edit: Got confused, thought you had removed the getSnapshot optimization. Never mind.
There was a problem hiding this comment.
No worries. This is something we can further optimize when we replace the two composed effects with our own pushed effect. Then we can just use one single effect and selectively unsubscribe/subscribe only when necessary.
Sounds like that will be a fun little cleanup refactor anyway. 😄
| !is(prevSource, source) || | ||
| !is(prevSubscribe, subscribe) || | ||
| !is(prevGetSnapshot, getSnapshot) | ||
| !is(prevSubscribe, subscribe) |
There was a problem hiding this comment.
This is neat that this is now the same condition that the effect hook uses when comparing deps. In the future we could "cheat" and read the deps off the effect hook instead of using a ref.
Pulls in @acdlite's failing tests from #18296.
Fixes one, leaves TODO comment for addressing the other two once we have a mechanism to do so. (Currently some temporary tearing is possible between passive effect flushes.)