Fixes #10065: fix race condition in useReactiveVar.

[Huulivoide]

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[apollo-cla]

@Huulivoide: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[benjamn]

@Huulivoide Thanks for catching this and fixing it!

I've pushed a couple of commits with changes that made sense to me, but I would appreciate your double-checking those changes to make sure they preserve the intent of the PR.

For example, is the early return safe when value !== probablySameValue (something I brought back)? Or was getting rid of that check and always calling setValue(rv()) important for your original implementation?

Also, it would be awesome if you can assemble any sort of regression test for this, so we don't break it again without noticing. 🙏

[benjamn]

I tried the current changes against your reproduction from #10065 (thanks for that!), and the counter appears to keep updating without getting stuck, so that's good news!

[Huulivoide]

I would drop the isSameValue check because it doesn't actually improve anything. It only makes the code more complex, and brings in a an additional bug vector.

React already does the same check internally https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update

Also the early return on line 20 breaks things badly. It is of utmost importance, that we always subscribe to the changes in the RV, which can now be skipped.

I added this verison to https://codesandbox.io/s/usereactivevar-gets-stuck-0hixe3?file=/src/App.js and it sometimes, not always, gets stuck in the very early phase.

[delph123]

Hello,

We just faced the same issue in our project with a counter counting up and down. So if I may intervene I want to do a few comments.

First of all, this is not exactly a race condition, actually the problem comes from the fact that the value of the react state may be different from the value of the reactive variable whenever the hook is triggered (due to react batching), hence, when the observer is set up again, the next change could notify the value of the react state and cause the issue (the reactive variable will update and notify observer then delete them, while the react state will bail out since it does not see any change).
To reproduce the issue you need to set different values in the reactive variable in the same batch and again the same value kind of values in another batch. Your unit test which only increases value therefore cannot reproduce the issue.

// What you can do instead is:
rv(rv()+1); rv(rv()+1);
setTimeout(() => { rv(rv()-1); rv(rv()-1) }, 100);

I did a simple POC based on this idea => https://github.com/delph123/apollo-usereactivevar-bug

I tend to agree with Huulivoide, the return on line 20 causes more issue in the implemation so it should be deleted.

As for the same value check, it is indeed not really relevant. With the proposed fix, all updates are notified and we could set the react state multiple times in same batch in lots of other situations (take my poc for example). Instead you could completely abstract the react state from the reactive variable and make the react state be a counter which will help avoid setState calls.
But as mentionned by Huulivoide react already does this and I think it's actually prefereable to delegate this task to react. Plus the original implementation works with a change of target reactive variable (ok - I can't find the use case but then again that's another bug I suppose).

I adapted my POC with the alternative implementation in case you want to play with it but again, on my side, I prefer the proposition of Huulivoide.

All the best!

[jerelmiller]

Hey @Huulivoide 👋

Appreciate the patience with this PR! I'd love to see if we can get this in a 3.8.x patch release in the near future.

I actually think we can fix this in a simpler way by using useSyncExternalStore. Here is what that would look like:

// use our polyfilled version of useSyncExternalStore
import { useSyncExternalStore } from './useSyncExternalStore'; 

function useReactiveVar<T>(rv: ReactiveVar<T>): T {
  return useSyncExternalStore(
    useCallback(
      (update) => {
        return rv.onNextChange(function onNext() {
          update();
          rv.onNextChange(onNext);
        });
      },
      [rv]
    ),
    () => rv(),
    () => rv(),
  );
}

I've tried this out locally and the test passes as expected.

@delph123 I've also tried this approach using a fork of your reproduction and the app behaves as expected.

@Huulivoide are you still interested in seeing this through? If not, I'd be happy to get this PR in a state thats mergeable.

[delph123]

Ah, yes! You're absolutely right, the useSyncExternalStore is probably the best fit for this use-case.

I'd love to see that land in 3.8.x branch. That would be a nice addition to useSuspenseQuery 👍

[netlify]

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	7b7f2b6

[changeset-bot]

🦋 Changeset detected

Latest commit: 7b7f2b6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jerelmiller]

Hey @Huulivoide 👋

Really appreciate the contribution here! I'd love to get this in our next patch release so I went ahead and made a couple updates to get this updated with the latest from main and switched to useSyncExternalStore.