Skip to content
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

useMutableSource: bugfix for new getSnapshot with mutation #18297

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 all 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
95 changes: 69 additions & 26 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 All @@ -998,19 +999,51 @@ function useMutableSource<Source, Snapshot>(
subscribe,
}: MutableSourceMemoizedState<Source, Snapshot>);

// Sync the values needed by our subscribe function after each commit.
// Sync the values needed by our subscription handler after each commit.
dispatcher.useEffect(() => {
refs.getSnapshot = getSnapshot;
}, [getSnapshot]);

// If we got a new source or subscribe function,
// we'll need to subscribe in a passive effect,
// and also check for any changes that fire between render and subscribe.
// Normally the dispatch function for a state hook never changes,
// 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Edit: Got confused, thought you had removed the getSnapshot optimization. Never mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// so we use a ref to ensure that it always has the latest version.
refs.setSnapshot = setSnapshot;

// Check for a possible change between when we last rendered now.
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, source, subscribe]);

// If we got a new source or subscribe function, re-subscribe in a passive effect.
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 +1060,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
Contributor

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

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

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

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 @@ -1042,15 +1077,6 @@ function useMutableSource<Source, Snapshot>(
}
}

// Check for a possible change between when we last rendered and when we just subscribed.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);
}
}

return unsubscribe;
}, [source, subscribe]);

Expand All @@ -1066,10 +1092,26 @@ 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.
if (
!is(prevGetSnapshot, getSnapshot) ||
!is(prevSource, source) ||
!is(prevSubscribe, subscribe) ||
!is(prevGetSnapshot, getSnapshot)
!is(prevSubscribe, subscribe)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

) {
// 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 +1129,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