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

Async/await causes a race condition in React #35

Open
bartvanremortele opened this issue Apr 16, 2021 · 5 comments · May be fixed by #37
Open

Async/await causes a race condition in React #35

bartvanremortele opened this issue Apr 16, 2021 · 5 comments · May be fixed by #37

Comments

@bartvanremortele
Copy link

When using useRecoilCallback and setting multiple atoms using the same atomEffect the current implementation breaks. It will cause a race condition getting the current localStorage value because of the async/await that was added to onSet and getState.

This bug causes you to lose state persisted in localStorage

@rhys-saldanha
Copy link
Contributor

Could you write a test that recreates this bug? Race conditions are flaky to test, but looking at example code will help explain the issue (which I think I've understood but I'm not 100% sure).

@lmartins
Copy link

lmartins commented May 5, 2021

I've encountered this Issue while looking to solve a problem I'm facing on my own app. My use case is that I have a list of preset values that I need to programmatically fill into several atoms of the same family.

Inside the useRecoilCallback, I'm looping through the array given out by the API and using set to fill in the atom value. The problem I was seeing was that only random atoms would get persisted to Local Storage, sometimes with incomplete values. Applying the change submitted by @bartvanremortele also fixed the issue for me, without unintended side-effects from what I could gather so far.

@bartvanremortele
Copy link
Author

Could you write a test that recreates this bug? Race conditions are flaky to test, but looking at example code will help explain the issue (which I think I've understood but I'm not 100% sure).

I'm sorry, I don't have time to write a test. This bug can be easily reproduced by updating two atoms in the same useRecoilCallback.

@stefanoTron
Copy link

#37 does fix this issue, but two tests fail for the async storage, which is to be expected since with the fix onSet does not take a async function anymore. I didn't go too deep but I don't see an easy solution except maybe adding a asyncStorage boolean flag to the config. 'asyncStorage` could default to true to avoid breaking changes... anyways I'm just brainstorming here, what do you think @polemius ?

@polemius
Copy link
Owner

I have fixed race condition for synchronize storage (like localStorage) in 2.7.0 version. But the problem still exist for async storage.

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 a pull request may close this issue.

5 participants