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] - Store Kit 1 race condition #2604

Merged
merged 62 commits into from
Jun 12, 2024

Conversation

arthurgeron
Copy link
Contributor

@arthurgeron arthurgeron commented Nov 8, 2023

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)

Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

@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!

@hyochan hyochan added 📱 iOS Related to iOS 🛠 bugfix All kinds of bug fixes 🍗 enhancement New feature or request labels Nov 10, 2023
@arthurgeron
Copy link
Contributor Author

@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 arthurgeron force-pushed the fix/storekit1-thread-safe branch 3 times, most recently from 8ae8d4d to 7c02787 Compare November 13, 2023 17:38
@arthurgeron
Copy link
Contributor Author

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

@Yann-prak
Copy link

Hi,

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

cervebar and others added 9 commits January 7, 2024 17:35
## docs fixes
- improve setup of project
- remove not working scripts and urls

- I managed to get Android example app to work, but unfortunately not
the iOS, see notes bellow:

## update `IAPExample` React to `18.2.0`
    
- because of `TypeError: undefined is not an object (evaluating
'ReactCurrentActQueue$1.isBatchingLegacy')`
 - see here: facebook/react-native#34079


## iOS exampke currently not buildable because of:
- because of obsolete pods:
 ```
  $ yarn pods
rbenv: pod: command not found

The `pod' command exists in these Ruby versions:
  2.7.4
```
- see here: facebook/react-native#38921

fixed but then build failed
```
The following build commands failed:
CompileC
/Users/babu/Library/Developer/Xcode/DerivedData/IapExample-ddzasbbwninhdjgvtldlfwsnfkwf/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/RCT-Folly.build/Objects-normal/arm64/json.o
/Users/babu/work/powerful-medical/dev/react-native-iap/IapExample/ios/Pods/RCT-Folly/folly/json.cpp
normal arm64 c++ com.apple.compilers.llvm.clang.1_0.compiler (in target
'RCT-Folly' from project 'Pods')
(1 failure)
``` 

-> this needs more time and investigation, and the question is, is it worth the time? - anyway added warning for anybody trying to build it regarding this README

---------

Co-authored-by: hyochan <[email protected]>
## Why

The current example project is somewhat outdated, making it challenging
for users to operate with the new SDK. They are required to use an older
iOS simulator and manage various legacy issues.

## How
Update the RN project to the latest version, `0.72.7`.

## Test Plan
<img
src="https://github.com/dooboolab-community/react-native-iap/assets/27461460/8df160e6-8e71-433c-ad6d-1e3d96851a1f"
width="320"/>
Run example projects in `iOS` and `Android`.

## Caveat
I'm trying to understand why react-native.config.js isn't effectively
embedding the local react-native-iap source code into the project. As of
now, I've manually installed react-native-iap in the package.json 🤔.
Reference:
https://github.com/react-native-community/cli/blob/main/docs/dependencies.md
- add renewal info (uncomment and serialize)
- change debugSerialize to serialize - there is info in this property
useful for production not only for development
- added transactionReasonIOS enum (to distinguish between type of
renewing transactions of purchase events)

see also:
[JWSTransactionDecodedPayload](https://developer.apple.com/documentation/appstoreserverapi/jwstransactiondecodedpayload)
- adding user canceled error code, so we can distinguish in app what
error is intention and what is real error
Update documentation according to Google’s latest requirements
@arthurgeron
Copy link
Contributor Author

@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
Copy link

hkeithk commented Apr 17, 2024

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

@hkeithk
Copy link

hkeithk commented Jun 3, 2024

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

@arthurgeron
Copy link
Contributor Author

arthurgeron commented Jun 4, 2024

@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 arthurgeron requested a review from hyochan June 4, 2024 15:11
@arthurgeron
Copy link
Contributor Author

@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
Copy link
Owner

hyochan commented Jun 11, 2024

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.

Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

GPT says it is perfect!
#2763 (comment)

@hyochan
Copy link
Owner

hyochan commented Jun 11, 2024

@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
Copy link
Contributor Author

arthurgeron commented Jun 11, 2024

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
Copy link
Contributor Author

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

@arthurgeron arthurgeron requested a review from hyochan June 11, 2024 23:36
@hyochan
Copy link
Owner

hyochan commented Jun 12, 2024

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

NP! It is working now :)

@hyochan hyochan merged commit 0a4236c into hyochan:main Jun 12, 2024
3 checks passed
Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

LGTM!

@Yann-prak
Copy link

Thanks guys ! You rock ! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠 bugfix All kinds of bug fixes 🍗 enhancement New feature or request 📱 iOS Related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS: Many crashes due to lack of thread-safety