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 demonstrating infinite updates #25179

Closed
wants to merge 1 commit into from

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Sep 2, 2022

This was.... fun?

So in a Next.js project there was an infinite updates issue causing a page to fail and unmount. After narrowing the scope of the problem as much as possible I was able to start to repro in a test case (see ReactDOMHooks-test.js)

There is a confluence of things happening

In StrictMode some effects are refired
In a SuspenseBoundary there is a OffscreenComponent fiber as a child fiber
DoubleInvoke-ing effects in Dev looks for whether there is a PlacementDEV flag or (critically) if the fiber is an OffscreenComponent.

This means that for every descendant of a SuspenseBoundary, you will double-invoke effects even on updates that happened after a Placement (In StrictMode of course)

So what is happening in this contrived test case is

a first layoutEffect is causing an immediate Sync render. there is also a passive effect queued to some non Sync lane.
a second layoutEffect is causing another immediate Sync render however b/c the passive effect is inside an OffscreenComponent it gets cleanedup and re-executed.

This effect has a setState but it cannot bail out of the update because there is pending work already on this fiber (from a previous run of the effect). It takes a few loops around but eventually there is an inifinite loop of setStates causing Sync renders causing setStates that cannot be bailed out, causing Sync renders...

At least I think...

The simple fix is probably to not always doubleinvoke with an OffscreenComponent child. I think that is wrong in the SuspenseBoundary case but maybe it makes sense for regular offscreen. I don't really know though

@sizebot
Copy link

sizebot commented Sep 2, 2022

Comparing: f0efa11...e9b2b5c

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 134.97 kB 134.97 kB = 43.23 kB 43.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 141.64 kB 141.64 kB = 45.20 kB 45.20 kB
facebook-www/ReactDOM-prod.classic.js = 486.06 kB 486.06 kB = 86.51 kB 86.51 kB
facebook-www/ReactDOM-prod.modern.js = 471.34 kB 471.34 kB = 84.29 kB 84.29 kB
facebook-www/ReactDOMForked-prod.classic.js +0.09% 486.06 kB 486.50 kB +0.15% 86.51 kB 86.64 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against e9b2b5c

@sammy-SC
Copy link
Contributor

Fixed by #25203

@gnoff thanks for the report and great repro!

@sammy-SC sammy-SC closed this Sep 12, 2022
@gnoff gnoff deleted the failing-test/infinite-updates branch January 11, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants