Skip to content

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 5, 2023

This is a bug originally extracted from internal tools at Meta. I further reduced it from the repro @kassens found and @rickhanlonii reduced in #26533. The fix was suggested by @acdlite.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 5, 2023
@acdlite
Copy link
Collaborator

acdlite commented Apr 5, 2023

To be clear I don’t think this is the right fix it just illustrates what the issue is. Which is that during the render phase we’re cloning a field (destroy) that is not thread safe.

The reason reusing the previous list of hooks doesn’t work in a bail out scenario is because the hook list also contains, aside from effects, the progressed set of state of queues. So if you don’t preserve those updates will end up firing multiple times.

The correct fix will need to change the data structure of effect “instances” so that they are thread safe. We were going to have to do this regardless to make Offscreen work because effects can be concurrent now: you can hide/show a tree at any time, even in the middle of another render phase.

@kassens
Copy link
Member

kassens commented Apr 6, 2023

Fixed by #26561

@kassens kassens closed this Apr 6, 2023
@kassens kassens deleted the fx-bug branch April 6, 2023 18:14
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.

5 participants