Fix useMemoCache with setState in render #30889
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
ghstack-source-id: b3e3b825955163045bc7a4f3df76b40263f728a0 Pull Request resolved: #30889
facebook-github-bot
added
CLA Signed
React Core Team
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: b3e3b825955163045bc7a4f3df76b40263f728a0 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: a71a7832188ef80b5c55c9f80b641282648ec735 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 62875e642be1f29b66354f812f06f8f0ce4b1b66 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 8bab4b4802284dc464a620b415bfb21b5ca52508 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 4c6350a60286b92113ee5922645406fa94b7b34a Pull Request resolved: #30889
…r on "Fix useMemoCache with setState in render" Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 5, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 4d25c920015ba7fb2d1014f01886baa6979e3d96 Pull Request resolved: #30889
josephsavona
commented
Sep 5, 2024
Comment on lines
638
to
642
| // Because we clone/reset the memo cache after every aborted attempt, we | ||
| // must process the first chunk again. | ||
| // | ||
| // That's three total times we've processed the first chunk, compared to | ||
| // just once when enableNoCloningMemoCache is on. |
There was a problem hiding this comment.
looks like this reset was just an artifact of the renderWithHooksAgain() reset. so the feature flag helps, but a bit less than we expected (we process the chunk once instead of twice)
There was a problem hiding this comment.
Hmm nope, more complicated than that. Tests seem to be failing on persistent mode with this change, still debugging.
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: d0508abb7babfcd7d96a166eabb4f8a7e19c9c54 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 57608c8a4d5ba86de26b9a4e1d3a9c4d5463b0e4 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: ab48eacccff535616dcd4a058637feb60d350380 Pull Request resolved: #30889
Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: 50bf0cebef9fbc1010a26f74ab5003be49df06e0 Pull Request resolved: #30889
…tate in render" Fixes the bug that alexmckenley and mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: fc0947ce219334117075df6a4e33b39975af2bc4 Pull Request resolved: #30889
josephsavona
commented
Sep 6, 2024
Comment on lines
+2523
to
+2531
| } | ||
| const lastEffect = componentUpdateQueue.lastEffect; | ||
| if (lastEffect === null) { | ||
| componentUpdateQueue.lastEffect = effect.next = effect; | ||
| } else { | ||
| const lastEffect = componentUpdateQueue.lastEffect; | ||
| if (lastEffect === null) { | ||
| componentUpdateQueue.lastEffect = effect.next = effect; | ||
| } else { | ||
| const firstEffect = lastEffect.next; | ||
| lastEffect.next = effect; | ||
| effect.next = firstEffect; | ||
| componentUpdateQueue.lastEffect = effect; | ||
| } | ||
| const firstEffect = lastEffect.next; | ||
| lastEffect.next = effect; | ||
| effect.next = firstEffect; | ||
| componentUpdateQueue.lastEffect = effect; |
There was a problem hiding this comment.
drive by cleanup since i was here
josephsavona
added a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: fc0947ce219334117075df6a4e33b39975af2bc4 Pull Request resolved: #30889
github-actions bot
pushed a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: fc0947ce219334117075df6a4e33b39975af2bc4 Pull Request resolved: #30889 DiffTrain build for commit 727b361.
github-actions bot
pushed a commit
that referenced
this pull request
Sep 6, 2024
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render. The fix here is to call a new helper function to clear the update queue. It nulls out other properties, but for memoCache it just sets the index back to zero. ghstack-source-id: fc0947ce219334117075df6a4e33b39975af2bc4 Pull Request resolved: #30889 DiffTrain build for [727b361](727b361)
This was referenced Sep 9, 2024
gnoff
pushed a commit
to vercel/next.js
that referenced
this pull request
Sep 12, 2024
**breaking change for canary users: Bumps peer dependency of React from `19.0.0-rc-7771d3a7-20240827` to `19.0.0-rc-94e652d5-20240912`** [diff facebook/react@7771d3a7...94e652d5](facebook/react@7771d3a...94e652d) <details> <summary>React upstream changes</summary> - facebook/react#30952 - facebook/react#30950 - facebook/react#30946 - facebook/react#30934 - facebook/react#30947 - facebook/react#30945 - facebook/react#30938 - facebook/react#30936 - facebook/react#30879 - facebook/react#30888 - facebook/react#30931 - facebook/react#30930 - facebook/react#30832 - facebook/react#30929 - facebook/react#30926 - facebook/react#30925 - facebook/react#30905 - facebook/react#30900 - facebook/react#30910 - facebook/react#30906 - facebook/react#30899 - facebook/react#30919 - facebook/react#30708 - facebook/react#30907 - facebook/react#30897 - facebook/react#30896 - facebook/react#30895 - facebook/react#30887 - facebook/react#30889 - facebook/react#30893 - facebook/react#30892 - facebook/react#30891 - facebook/react#30882 - facebook/react#30881 - facebook/react#30870 - facebook/react#30849 - facebook/react#30878 - facebook/react#30865 - facebook/react#30869 - facebook/react#30875 - facebook/react#30800 - facebook/react#30762 - facebook/react#30831 - facebook/react#30866 - facebook/react#30853 - facebook/react#30850 - facebook/react#30847 - facebook/react#30842 - facebook/react#30837 - facebook/react#30848 - facebook/react#30844 - facebook/react#30839 - facebook/react#30802 - facebook/react#30841 - facebook/react#30827 - facebook/react#30826 - facebook/react#30825 - facebook/react#30824 - facebook/react#30840 - facebook/react#30838 - facebook/react#30836 - facebook/react#30819 - facebook/react#30816 - facebook/react#30814 - facebook/react#30813 - facebook/react#30812 - facebook/react#30811 </details> --------- Co-authored-by: vercel-release-bot <infra+release@vercel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Fixes the bug that @alexmckenley and @mofeiZ found where setState-in-render can reset useMemoCache and cause an infinite loop. The bug was that renderWithHooksAgain() was not resetting hook state when rerendering (so useMemo values were preserved) but was resetting the updateQueue. This meant that the entire memo cache was cleared on a setState-in-render.
I initially changed renderWithHooksAgain() to just reset the updateQueue, nulling out properties and setting the memoCache.index back to 0, but not clearing memoCache.data. That fixed the uMC bug, but caused some tests to fail where effect cleanup wasn’t running when a component was in a tree that suspended and showed a fallback.
Turns out renderWithHooksAgain() is also used to re-render suspended components, and that case depends on the updateQueue being cleared for effects to work correctly. So i updated to clear the updateQueue for the suspense case, reset in the setState-in-render case, and everything works. The result feels pretty clean.
In a follow-up we should be able to avoid resetting the memo cache in the replay case as well. There are a few spots in the codebase where we check for
updateQueue === null, and only do processing in that case — including some code related to effect cleanup. I think the right thing to do there would be to check for e.g.updateQueue?.lastEffect == null(and any other relevant properties from updateQueue), and then it would be safe to always reset the updateQueue instead of clearing it.