[Fix] - Store Kit 1 race condition

[arthurgeron]

Fixes: #1696, #2372

Similar to #2518, this fixes race condition crashes on validProducts and productsRequest in RNIapIos.swift when using Store Kit 1.

React Native IAP

What was done:

• Created ThreadSafe class; Which now wraps validProducts.
• Created LatestPromiseKeeper;
  • Only maintains latest promise, fixes racing condition when multiple productsRequest are made in parallel. By resolving only one promise at any given time (only for productsRequest), we eliminate any possibility of causing racing conditions or bad exec exceptions while storing the resolved data.
  • If more than one promise is made, older promises will be discarded and return a rejection.

IAPExample (iOS)

• Fixed target iOS deployment being loaded from a undefined variable, now, it fetches the project's target iOS from the Xcode project and applies it during pod install
• Updated GitHub Action to use Node.js 20
• Changed GitHub Action runner from M1 arch to Intel based macOS (fixes crash during build)

[arthurgeron]

@arthurgeron Could you kindly check on the CI failing before going further? https://github.com/dooboolab-community/react-native-iap/actions/runs/6801993699/job/18494078412

Thanks in advance!

Thank you for your patience. I've fixed the SwiftLint errors!

[arthurgeron]

@hyochan I'm not sure why the iapExample ios build is failing.

[Yann-prak]

Hi,

Do you need some help to get through this PR ? What's missing ? :)

[arthurgeron-work]

Hi,

Do you need some help to get through this PR ? What's missing ? :)

Hi Yann, I don't know why the iapExample GitHub action is failing; the logs don't tell much aside from the error being in LatestPromiseKeeper.swift. I haven't been able to reproduce it, and the example project on this repository is outdated, so it can't be used to test. Xcode doesn't show any issues, nor does swift lint.

The issue is probably happening because the example project is outdated.

[LcsGrz]

Hi guys, is there any news about this PR? :D

We are waiting for it, thank you very much for your time and dedication

[arthurgeron-work]

Hi guys, is there any news about this PR? :D

We are waiting for it, thank you very much for your time and dedication

It might some time to fix the example project as it's quite outdated. But the issue itself is fixed and working at this branch, which means you can safely bring the changes to your project via patch package, cheers!

[LcsGrz]

@arthurgeron-work

Hi, hope you are well :D

I was testing your branch and the truth is that it works very well! no crashes! But I found a problem, when you do 'getSubscriptions' (I use the hook of useIAP) there are several occasions where the promise is not resolved.

For example, the same function is executed 4 times in a row, and 2 of the 4 get a response, the others are never resolved neither by error nor as sastifactory.

I found all this from the react native code, I don't know how to debug in swift because I don't know the language well so I can't help you.

I understand that it executes 4 times my code is a problem on my side, but it could happen that in the first execution the promise is not solved anyway.

I leave an example of what I do

  const handleGetSubscriptions = async (skus: string[]) => {
    setLoading(true);

    try {
      await getSubscriptions({ skus });
    } catch (error) {
      logError('handleGetSubscriptions', error);
    }

    setLoading(false);
  };

  const SKU = useRemoteConfigValue(remoteConfigSkuKey);

  useEffect(() => {
    if (connected && SKU) {
      handleGetSubscriptions([SKU]);
    }
  }, [connected, SKU]);

[arthurgeron]

I understand that it executes 4 times my code is a problem on my side, but it could happen that in the first execution the promise is not solved anyway.

Promises work a little different in Swift and can be canceled. The original problem surged because it couldn't handle parallel getProducts/Subs calls but it was never meant to support that.

The promise does not return any object, instead, the property subscriptions returned by useIAP is eventually updated after the promise is resolved internally. This means you should make the fetchSubs call only once and then wait for an update on the subscriptions property from useIAP via useEffect. Or if for some reason it's absolutely necessary to make the call more than once, your code should be aware and ignore/discard older promises, you can use useEffect's cleanup API to cancel your own async methods that call getSubscription if necessary.

Allowing more than one promise to finish executing at the same time could be a problem for several reasons, one being many react state/render updates for the same return object, an older promise resolving after the most recent one, concurrency, etc. As I have commented out in the added code, it intentionally cancels older promises for the same class (e.g. products, subscription) to avoid these issues, also returning a false reject/resolve from canceled promises would generate another set of issues.

[arthurgeron]

To further exemplify, here's how we deal with that:

const StoreSKUsToFetch = ['someSku'];

function useSetupIAP() {

  /* useIAP will replace old product data with each new `getProduct`or similar call
   * so we need to consolidate all possible solutions once, other hooks have the responsability of filtering/selecting data
   */
  useEffect(() => {
    if (!fetchedProductsOnce && StoreSKUsToFetch?.length) {
      getSubscriptions({ skus: StoreSKUsToFetch })
        .then(() => {
          setFetchedProductsOnce(true);
        })
        .catch((e) => console.error('failed to fetch IAP data', e));
    }
  }, [fetchedProductsOnce, getSubscriptions]);
}

This hook is used only once in a HOC.

[LcsGrz]

Thank you very much, I found your explanation and advice very useful, very well explained 🔥🤙🏻

[LcsGrz]

I've been thinking about this for a while Shouldn't this change to 'cancel' the previous promises be announced breaking change?

And for example, when a promise 'is cancelled' isn't it possible to return a result/status to the javascript code?

because for example something like this

await getSubscriptions({ skus });

will stay forever waiting for the promise to be resolved, it will never continue with the code execution, and maybe if several people are using something like this, they will have problems with this new version if this PR is merged

Do you understand what I am explaining? I'm not always good at this, but I want to be clear hehe

[arthurgeron]

Don't worry you're being very clear and I understand what you mean, I think it'd indeed make sense to reject older/discarded promises to signal cancellation just to avoid hanging async methods, though it'll still "break" for anyone doing parallel requests.

I'll look into it when possible, thanks for the suggestions and for now avoid parallel requests when fetching products/subs!

[openpathgit]

Would love to see this pushed to a release, these crashes are causing us a lot of heartache

[arthurgeron]

I was without a functioning MacBook since Jan/February, so it took me a while to circle back to this. @LcsGrz I've updated my branch with main and implemented Promise rejection, so your case should now be covered! @hyochan Github Action is failing on building IAPExample for iOS, I was able to build locally, using the exact same command, though only with NO_FLIPPER=1 set in my env.

I've tried:

• Updating it to use Node.JS 20
• Setting it to use default ruby v2.6.10
• Fixing mismatch between the project's minimum ios version and Pod's target ios

Nothing seems to work, and the logs don't tell much aside from that it failed to build LatestPromiseKeeper.swift and RNIapIos.swift, even though Xcode builds locally and doesn't show any warnings or errors for the file.

Are you able to get more detailed logs? I'm on a loss on where to go from here. Thank you

[openpathgit]

Really excited for this fix! @hyochan @LcsGrz please help :)

[arthurgeron]

@hyochan I've figured out the issue with IAPExample's build on GitHub Action for iOS. Apparently there's some architecture specific issue when running with macos-latest, which uses Apple Silicon (M1), I wasn't able to consistently reproduce it on my M3 (even with Act). Switching it to Intel macOS has fixed the issue.

I took the liberty of adjusting other factors that generated some issues during my testing.

Let me know if there's anything else necessary for this to get this merged.

[hkeithk]

Hi @hyochan, sorry to bother, was hoping you may be able to take a look at this when you have some time. Thanks!!

[hkeithk]

@hyochan @andresesfm just want to give this a final try, hoping we can get some eyes on this, thank you so much

[arthurgeron]

@hyochan @andresesfm just want to give this a final try, hoping we can get some eyes on this, thank you so much

While not ideal, you can use patch-package to test and apply this fix to your project. Just replace the files in node_modules/react-native-iap/ios and run patch-package react-native-iap, it'll generate a diff file for the changes, then you can set it up to auto-apply the fix after each install.

[arthurgeron]

@hyochan Personally, I'd prefer if calls like getProducts returned the data directly in the same Promise that is generated, instead of storing it internally and returning from a separate variable. It would delegate the responsibility of storing the data to the developer, allow for agnostic data managers (e.g. React-Query, Redux, Zustand, etc) and avoid all these issues with Racing Conditions and Thread Safety, as well as being much easier to maintain. This is a little bit off-topic and there might be deeper reasons as to why it was made this way, but I just wanted to give my 5 cents on it anyway.

[hyochan]

Personally, I'd prefer if calls like getProducts returned the data directly in the same Promise that is generated, instead of storing it internally and returning from a separate variable. It would delegate the responsibility of storing the data to the developer, allow for agnostic data managers (e.g. React-Query, Redux, Zustand, etc) and avoid all these issues with Racing Conditions and Thread Safety, as well as being much easier to maintain. This is a little bit off-topic and there might be deeper reasons as to why it was made this way, but I just wanted to give my 5 cents on it anyway.

@arthurgeron I apologize for the delayed code review. If I understood your comment correctly, you want to handle getProducts using a Promise-based approach. In the past, I have implemented this library with Promises and encountered critical issues, which I have previously documented in a blog post: https://medium.com/dooboolab/react-native-iap-v3-1259e0b0c017. Promises return only one result per command, making it impossible to handle all events in the event loop. Although I have maintained some parts of the existing Promise-based implementation, the need to return results from multiple events remains critical to avoid missing any events. This approach might actually be more appropriate for the responsibility delegation you mentioned ("It would delegate the responsibility of storing the data to the developer"). If I have misunderstood your point, please feel free to correct me.

Once again, I apologize for the delay and any inconvenience caused.

[hyochan]

@arthurgeron Could you kindly check this? Then I can quickly merge this and deploy to newer version! https://github.com/dooboolab-community/react-native-iap/actions/runs/9469799195/job/26089289144?pr=2604

[arthurgeron]

Welcome back @hyochan! Swift lint issues are now fixed. Your explanation about the reason behind how data is delivered makes sense, I appreciate your detailed explanation!

[arthurgeron]

@hyochan CI issues fixed but Github Actions is acting up 😅

[hyochan]

@hyochan CI issues fixed but Github Actions is acting up 😅

NP! It is working now :)

[Yann-prak]

Thanks guys ! You rock ! 🚀