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

Fix race condition when tempState is deleted before loading flag is set to false #376

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

ospfranco
Copy link
Contributor

Details

There is a small race condition. When calling setWithOnyxState we delete the tempState object that is used to hold temporary state updates while the HOC is gathering data. However the loading flag is only set on the setState callback down below. setState however is an async operation that is also batched, therefore there is a small window of time where the loading flag is not false but the tempState is already gone (while the update is queued and waiting to be applied). This simply bypasses the loading check if the tempState is gone and the update can be safely queued with a normal setStateProxy.

Related Issues

GH_LINK Expensify/App#28419

Automated Tests

Linked PRs

@ospfranco ospfranco requested a review from a team as a code owner September 29, 2023 06:44
@melvin-bot melvin-bot bot requested review from cristipaval and removed request for a team September 29, 2023 06:45
mountiny
mountiny previously approved these changes Sep 29, 2023
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good but I ma not fan of the line numbers in the comment, that will inevitably change and become out of date. Could you please rewrite it without the numbers

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks!

@mountiny mountiny merged commit 2ae80f9 into Expensify:main Sep 29, 2023
@ospfranco ospfranco deleted the osp/fix-concurrency-with-set-state branch September 29, 2023 07:14
@getusha
Copy link

getusha commented Sep 29, 2023

@mountiny we were also attempting to get this change merged here #368 😄, anyway thanks for handling it.

@mountiny
Copy link
Contributor

mountiny commented Oct 2, 2023

Ah yeah, saw that, this was faster for our purposes :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants