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.

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

Failing test case for useMutableSource #18296

Closed
wants to merge 3 commits into from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 13, 2020

Scenario: getSnapshot changes during render. The render finishes and commits. But before the passive effects fire, the source is mutated again. The subscription still thinks the old getSnapshot is current.

I think this means that whenever getSnapshot changes, we need to check if there was a mutation since render. Similar to what we do when resubscribing.

@acdlite acdlite changed the title Failing test case for useMutableSource. Failing test case for useMutableSource Mar 13, 2020
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 13, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 13, 2020

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 371c14e:

Sandbox Source
spring-cache-e35ei Configuration

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

Nice! Thanks for the test case. I'll fix this in the morning.

@sizebot
Copy link

sizebot commented Mar 13, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 371c14e

@sizebot
Copy link

sizebot commented Mar 13, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 371c14e

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

Thanks again!

#18297

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 13, 2020

@bvaughn I pushed another test case. Mind taking a look at that, too?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

On it.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 13, 2020

Also I probably shouldn't have written the description as if this were specific to passive effects, since it applies to a mutation in an interleaved event during render, too.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 13, 2020

I think what the second test case indicates is that we aren't resetting the queue properly here:

stateHook.baseQueue = null;
snapshot = readFromUnsubcribedMutableSource(root, source, getSnapshot);
stateHook.memoizedState = stateHook.baseState = snapshot;

That clears the updates, but what we also need to do is create an entire new queue object and re-bind the setState method. So that even if there are interleaved updates, they get pushed to the older queue.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

Yeah, I was noticing that.

Thanks for the pointer 😄

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 13, 2020

More concretely:

const newQueue = {
  pending: null,
  dispatch: null,
  lastRenderedReducer: basicStateReducer,
  lastRenderedState: whatever,
};
newQueue.dispatch = dispatchAction.bind(currentlyRenderingFiber, newQueue);

// Now when this becomes current, the previous queue and dispatch method
// are complete discarded, including any interleaving updates that occur.
hook.queue = newQueue;

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

😆 Yes, thanks...

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

From my investigation, the effect that updates the getSnapshot ref does run and queue a state update with the correct value- but the earlier one in handleChange wins out with the old getSnapshot value. So disconnecting the old queue/dispatch function seems reasonable.

It doesn't fix this case though, and it also breaks several others 😅so I need to dig some more.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

It's interesting that removing the first hook "fixes" the problem above for the second hook.

I commented out the first hook temporarily (and hard-coded the value of first) just to reduce noise in my logs, and the final state of second after the new dispatcher and mutation is now correct. The state update in useEffect that syncs getSnapshot gets applied correctly.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

One noticeable difference is eager state computation. Not sure if it's relevant. It only happens for the first state hook on a given fiber that schedules an update. So if second is the only one, its state gets eagerly evaluated during the mutation. If first is the first state hook to update, second state doesn't get eagerly snapshot.

Update It seems the eager comparison prevents an update from being scheduled on the fiber when the change handler gets called for second (because the values are the same). When the getSnapshot syncing effect later updates, we schedule another state update. But if the eager comparison doesn't get run, we schedule an update initially - and then another update when the getSnapshot syncing effect runs, at a lower expiration time - but that update is ignored when the component re-renders.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 13, 2020

I think I can write a similarly broken test case with just the state hook, but maybe I'm misunderstanding something.

it("repro", async () => {
  let setStateA;
  let setStateB;

  function App({ count }) {
    const [a, setA] = React.useState("initial-a");
    const [b, setB] = React.useState("initial-b");
    React.useEffect(() => {
      if (count === 1) {
        setA("cascading-a");
        setB("cascading-b");
      }
    }, [count]);
    setStateA = setA;
    setStateB = setB;
    return `${a}:${b}`;
  }

  const root = ReactNoop.createRoot();
  await act(async () => {
    root.render(<App count={0} />);
  });

  await act(async () => {
    root.render(<App count={1} />);
    // Render and finish the tree, but yield before passive effects fire.
    expect(Scheduler).toFlushUntilNextPaint([]);
    // This is at high priority so that it doesn't get batched with default
    // priority updates that might fire during the passive effect
    ReactNoop.discreteUpdates(() => {
      setStateB("high-pri-b");
    });
    expect(Scheduler).toFlushUntilNextPaint([]);
    expect(root.getChildrenAsJSX()).toEqual("cascading-a:high-pri-b");
  });
});

Rather than a final state of "cascading-a:high-pri-b", the above results in a final state of "initial-a:high-pri-b".

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 16, 2020

The test you wrote is correct. I think the confusion is because there's another paint after that one, at normal priority, that does the cascading updates. Here's that same test with more assertions added:

it('repro', async () => {
  let setStateA;
  let setStateB;

  function App({count}) {
    const [a, setA] = React.useState('initial-a');
    const [b, setB] = React.useState('initial-b');
    React.useEffect(() => {
      if (count === 1) {
        setA('cascading-a');
        setB('cascading-b');
      }
    }, [count]);
    setStateA = setA;
    setStateB = setB;
    return `${a}:${b}`;
  }

  const root = ReactNoop.createRoot();
  await act(async () => {
    root.render(<App count={0} />);
  });

  await act(async () => {
    root.render(<App count={1} />);
    // Render and finish the tree, but yield before passive effects fire.
    expect(Scheduler).toFlushUntilNextPaint([]);
    expect(root).toMatchRenderedOutput('initial-a:initial-b');
    // This is at high priority so that it doesn't get batched with default
    // priority updates that might fire during the passive effect
    ReactNoop.discreteUpdates(() => {
      setStateB('high-pri-b');
    });
    expect(Scheduler).toFlushUntilNextPaint([]);
    // High pri updates have finished, but not the cascading ones,
    // which have normal pri
    expect(root).toMatchRenderedOutput('initial-a:high-pri-b');
  });
  // Now the final state has rendered, including the cascading updates
  expect(root).toMatchRenderedOutput('cascading-a:cascading-b');
});

@bvaughn
Copy link
Contributor

bvaughn commented Mar 16, 2020

I see. Thanks for clarifying.

I wonder why I'm seeing the update clobber in the way I'm seeing for uMS's internal state hook.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 17, 2020

Okay, looking at this again today.

Disconnecting dispatch from the update queue works to prevent the old B selector from overriding the new A selector value when A is mutated- but the mutation to A is also missed temporarily (since the subscribe function is still using the previous getSnapshot at this point).

The effect that updates getSnapshot sees the change, and uses the newly bound dispatchAction method to schedule the "missed" update, but when the component renders- the "missed" update is scheduled at an insufficient expiration time, so it's skipped for that update (and tearing happens for A0 and A1).

I think the getSnapshot effect needs to schedule its missed update at a higher priority, to make sure it's included in the next render- that or just replace the intermediate update entirely (but that seems shady).

Scenario: `getSnapshot` changes during render. The render finishes and
commits. But before the passive effects fire, the source is mutated
again. The subscription still thinks the old `getSnapshot` is current.
Not sure the best fix for this one.
@acdlite acdlite closed this Mar 20, 2020
@acdlite
Copy link
Collaborator Author

acdlite commented Mar 20, 2020

Gah I did it again!

@acdlite acdlite reopened this Mar 20, 2020
@acdlite
Copy link
Collaborator Author

acdlite commented Mar 20, 2020

@bvaughn

I think the getSnapshot effect needs to schedule its missed update at a higher priority, to make sure it's included in the next render- that or just replace the intermediate update entirely (but that seems shady).

I think that's the right idea, except it's not only limited to high priority updates. It could be a lower priority update, too. (I pushed another test case to cover that.)

What's tricky is there could be multiple pending mutations at different priorities. None of them should be allowed to commit without also committing the new update.

We've been using the word "entanglement" to describe this behavior. We don't have a mechanism to do this without de-opting, currently, but I'm planning to add one as part of the useTransition improvements. (This is the "terminal updates" problem.)

In the meantime, I think we should synchronously flush the pending mutation updates. Then we can do the entanglement thing once that exists.

I'll push a commit to show what I mean. Nvm, it's only one line so I'll post here instead:

import {markRootExpiredAtTime} from './ReactFiberRoot';
markRootExpiredAtTime(root.mutableSourcePendingUpdateTime);

);
});
expect(Scheduler).toHaveYielded([
'TODO: This currently tears. Fill in with correct values once bug',
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 this was meant to be a comment? 😄

@bvaughn
Copy link
Contributor

bvaughn commented Mar 20, 2020

Thanks for the added context, Andrew.

Using markRootExpiredAtTime means that tests two and three will temporarily tear. (I'm actually not seeing any difference in behavior for the tests whether I use markRootExpiredAtTime or not.)

I've pushed an update to #18297 though.

Edit I wonder if it's worth considering pushing a layout effect when getSnapshot changes, to avoid the temporary tear being visible? This would require some changes to the way the hook is implemented.

@acdlite acdlite closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants