Skip to content

useMutableSource: bugfix for new getSnapshot with mutation #18297

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 3, 2020
Merged

useMutableSource: bugfix for new getSnapshot with mutation #18297

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bee11d
useMutableSource: bugfix for new getSnapshot with mutation
Mar 31, 2020
4d25d4e
Split mutable source pending expiration times into first and last
Apr 2, 2020
1a73af5
Fixed remaining tearing cases
Apr 2, 2020
db511f4
Swapped first/last logic in setPendingExpirationTime and removed no l…
Apr 3, 2020
86da5ce
Recreate queue and dispatch when source/subscribe/getSnapshot change
Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 93 additions & 11 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type {
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {enableUseEventAPI} from 'shared/ReactFeatureFlags';

import {markRootExpiredAtTime} from './ReactFiberRoot';
import {NoWork, Sync} from './ReactFiberExpirationTime';
import {readContext} from './ReactFiberNewContext';
import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents';
Expand Down Expand Up @@ -74,7 +75,7 @@ import {
getCurrentPriorityLevel,
} from './SchedulerWithReactIntegration';
import {
getPendingExpirationTime,
getLastPendingExpirationTime,
getWorkInProgressVersion,
markSourceAsDirty,
setPendingExpirationTime,
Expand Down Expand Up @@ -886,6 +887,7 @@ function rerenderReducer<S, I, A>(
type MutableSourceMemoizedState<Source, Snapshot> = {|
refs: {
getSnapshot: MutableSourceGetSnapshotFn<Source, Snapshot>,
setSnapshot: Snapshot => void,
},
source: MutableSource<any>,
subscribe: MutableSourceSubscribeFn<Source, Snapshot>,
Expand Down Expand Up @@ -914,16 +916,14 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
isSafeToReadFromSource = currentRenderVersion === version;
} else {
// If there's no version, then we should fallback to checking the update time.
const pendingExpirationTime = getPendingExpirationTime(root);
const pendingExpirationTime = getLastPendingExpirationTime(root);

if (pendingExpirationTime === NoWork) {
isSafeToReadFromSource = true;
} else {
// If the source has pending updates, we can use the current render's expiration
// time to determine if it's safe to read again from the source.
isSafeToReadFromSource =
pendingExpirationTime === NoWork ||
pendingExpirationTime >= renderExpirationTime;
isSafeToReadFromSource = pendingExpirationTime >= renderExpirationTime;
}

if (isSafeToReadFromSource) {
Expand Down Expand Up @@ -972,7 +972,8 @@ function useMutableSource<Source, Snapshot>(

const dispatcher = ReactCurrentDispatcher.current;

const [currentSnapshot, setSnapshot] = dispatcher.useState(() =>
// eslint-disable-next-line prefer-const
let [currentSnapshot, setSnapshot] = dispatcher.useState(() =>
readFromUnsubcribedMutableSource(root, source, getSnapshot),
);
let snapshot = currentSnapshot;
Expand Down Expand Up @@ -1000,7 +1001,49 @@ function useMutableSource<Source, Snapshot>(

// Sync the values needed by our subscribe function after each commit.
dispatcher.useEffect(() => {
const didGetSnapshotChange = !is(refs.getSnapshot, getSnapshot);
refs.getSnapshot = getSnapshot;

// Normally the dispatch function for a state hook never changes,
// but in the case of this hook, it will change if getSnapshot changes.
// 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.
refs.setSnapshot = setSnapshot;

// This effect runs on mount, even though getSnapshot hasn't changed.
// In that case we can avoid the additional checks for a changed snapshot,
// because the subscription effect below will cover this.
if (didGetSnapshotChange) {
// Because getSnapshot is shared with subscriptions via a ref,
// we don't resubscribe when getSnapshot changes.
// This means that we also don't check for any missed mutations
// between the render and the passive commit though.
// So we need to check here, just like when we newly subscribe.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);

const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
fiber,
suspenseConfig,
);
setPendingExpirationTime(root, expirationTime);

// If the source mutated between render and now,
// there may be state updates already scheduled from the old getSnapshot.
// Those updates should not commit without this value.
// There is no mechanism currently to associate these updates though,
// so for now we fall back to synchronously flushing all pending updates.
// TODO: Improve this later.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
}
}
}
}, [getSnapshot]);

// If we got a new source or subscribe function,
Expand All @@ -1009,8 +1052,10 @@ function useMutableSource<Source, Snapshot>(
dispatcher.useEffect(() => {
const handleChange = () => {
const latestGetSnapshot = refs.getSnapshot;
const latestSetSnapshot = refs.setSnapshot;

try {
setSnapshot(latestGetSnapshot(source._source));
latestSetSnapshot(latestGetSnapshot(source._source));

// Record a pending mutable source update with the same expiration time.
const currentTime = requestCurrentTimeForUpdate();
Expand All @@ -1027,9 +1072,11 @@ function useMutableSource<Source, Snapshot>(
// e.g. it might try to read from a part of the store that no longer exists.
// In this case we should still schedule an update with React.
// Worst case the selector will throw again and then an error boundary will handle it.
setSnapshot(() => {
throw error;
});
latestSetSnapshot(
Copy link
Collaborator

@acdlite acdlite Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to do the same markRootExpiredAtTime thing during initial mount and when resubscribing, too, to avoid a momentary flicker.

Copy link
Contributor Author

@bvaughn bvaughn Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@bvaughn bvaughn Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

(() => {
throw error;
}: any),
);
}
};

Expand All @@ -1048,6 +1095,20 @@ function useMutableSource<Source, Snapshot>(
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);

const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
fiber,
suspenseConfig,
);
setPendingExpirationTime(root, expirationTime);

// 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));
Copy link
Collaborator

@acdlite acdlite Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to mark this new setSnapshot call as a pending mutation, too, right before marking it as expired.

Copy link
Contributor Author

@bvaughn bvaughn Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense conceptually, but it breaks the most recent test case I added.

In this new test I...

  1. Mount and flush component A with data version 1.
  2. Mount component B with data version 1 (but flush passive effects).
  3. Mutate the data (which schedules an update for already mounted component A.
  4. Flush passive effects (which also schedules an update for the newly mounted component B).
  5. 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).

Copy link
Contributor Author

@bvaughn bvaughn Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to push this change as a separate commit (rather than squashing) just to give you the opportunity to review it more closely: 7c9181f

Back to you @acdlite

}
}

Expand All @@ -1065,11 +1126,31 @@ function useMutableSource<Source, Snapshot>(
//
// In both cases, we need to throw away pending udpates (since they are no longer relevant)
// and treat reading from the source as we do in the mount case.
const didGetSnapshotChange = !is(prevGetSnapshot, getSnapshot);
if (
!is(prevSource, source) ||
!is(prevSubscribe, subscribe) ||
!is(prevGetSnapshot, getSnapshot)
didGetSnapshotChange
) {
if (didGetSnapshotChange) {
Copy link
Collaborator

@acdlite acdlite Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this nested check. Need to drop updates from new sources, too.

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure about that... I'll remove it 😄

Thanks for the review!

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@acdlite acdlite Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see how to fix it. Will let me simplify things a bit too...

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@acdlite acdlite Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Create a new queue and setState method,
// So if there are interleaved updates, they get pushed to the older queue.
// When this becomes current, the previous queue and dispatch method will be discarded,
// including any interleaving updates that occur.
const newQueue = {
pending: null,
dispatch: null,
lastRenderedReducer: basicStateReducer,
lastRenderedState: snapshot,
};
newQueue.dispatch = setSnapshot = (dispatchAction.bind(
null,
currentlyRenderingFiber,
newQueue,
): any);
stateHook.queue = newQueue;
}

stateHook.baseQueue = null;
snapshot = readFromUnsubcribedMutableSource(root, source, getSnapshot);
stateHook.memoizedState = stateHook.baseState = snapshot;
Expand All @@ -1087,6 +1168,7 @@ function mountMutableSource<Source, Snapshot>(
hook.memoizedState = ({
refs: {
getSnapshot,
setSnapshot: (null: any),
},
source,
subscribe,
Expand Down
5 changes: 3 additions & 2 deletions packages/react-reconciler/src/ReactFiberRoot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ type BaseFiberRootProperties = {|
lastExpiredTime: ExpirationTime,
// Used by useMutableSource hook to avoid tearing within this root
// when external, mutable sources are read from during render.
mutableSourcePendingUpdateTime: ExpirationTime,
mutableSourceLastPendingUpdateTime: ExpirationTime,
|};

// The following attributes are only used by interaction tracing builds.
Expand Down Expand Up @@ -130,7 +130,8 @@ function FiberRootNode(containerInfo, tag, hydrate) {
this.nextKnownPendingLevel = NoWork;
this.lastPingedTime = NoWork;
this.lastExpiredTime = NoWork;
this.mutableSourcePendingUpdateTime = NoWork;
this.mutableSourceFirstPendingUpdateTime = NoWork;
this.mutableSourceLastPendingUpdateTime = NoWork;

if (enableSchedulerTracing) {
this.interactionThreadID = unstable_getThreadID();
Expand Down
18 changes: 13 additions & 5 deletions packages/react-reconciler/src/ReactMutableSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,28 @@ export function clearPendingUpdates(
root: FiberRoot,
expirationTime: ExpirationTime,
): void {
if (root.mutableSourcePendingUpdateTime <= expirationTime) {
root.mutableSourcePendingUpdateTime = NoWork;
if (expirationTime <= root.mutableSourceLastPendingUpdateTime) {
// All updates for this source have been processed.
root.mutableSourceLastPendingUpdateTime = NoWork;
}
}

export function getPendingExpirationTime(root: FiberRoot): ExpirationTime {
return root.mutableSourcePendingUpdateTime;
export function getLastPendingExpirationTime(root: FiberRoot): ExpirationTime {
return root.mutableSourceLastPendingUpdateTime;
}

export function setPendingExpirationTime(
root: FiberRoot,
expirationTime: ExpirationTime,
): void {
root.mutableSourcePendingUpdateTime = expirationTime;
const mutableSourceLastPendingUpdateTime =
root.mutableSourceLastPendingUpdateTime;
if (
mutableSourceLastPendingUpdateTime === NoWork ||
expirationTime < mutableSourceLastPendingUpdateTime
) {
root.mutableSourceLastPendingUpdateTime = expirationTime;
}
}

export function markSourceAsDirty(mutableSource: MutableSource<any>): void {
Expand Down
Loading