fix(ios): resolve fetchProducts 30-second timeout#19
Conversation
Replace synchronous semaphore-based blocking pattern with async Task + signal emission, matching the pattern used by requestPurchase and other iOS methods. The semaphore was causing thread starvation deadlocks, resulting in 30-second timeouts. Closes #15 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update GDScript wrapper to use async signal-based flow for iOS fetchProducts instead of expecting synchronous return. Android remains unchanged (synchronous). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical deadlock issue in the iOS implementation of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConverts iOS product fetching to an asynchronous, signal-driven flow: the native iOS plugin returns immediately with a pending status and emits a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GodotIAP
participant SwiftPlugin
participant AppStore
rect rgba(200,150,100,0.5)
Note over Client,AppStore: Previous Behavior (Synchronous - Blocking)
Client->>GodotIAP: fetch_products(request)
GodotIAP->>SwiftPlugin: fetchProducts(json)
SwiftPlugin->>SwiftPlugin: create semaphore / wait
SwiftPlugin->>AppStore: async query products
AppStore-->>SwiftPlugin: products result
SwiftPlugin->>SwiftPlugin: build JSON response
SwiftPlugin-->>GodotIAP: return JSON string (blocking)
GodotIAP-->>Client: return result
end
rect rgba(100,200,150,0.5)
Note over Client,AppStore: New Behavior (Asynchronous - Signal-based)
Client->>GodotIAP: fetch_products(request)
GodotIAP->>GodotIAP: await _fetch_products_raw
GodotIAP->>SwiftPlugin: fetchProducts(json)
SwiftPlugin-->>GodotIAP: return pending status (immediate)
SwiftPlugin->>AppStore: async query products (non-blocking)
AppStore-->>SwiftPlugin: products result
SwiftPlugin-->>GodotIAP: emit productsFetched signal
GodotIAP->>GodotIAP: await signal received
GodotIAP->>GodotIAP: build and return products/error dictionary
GodotIAP-->>Client: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the 30-second timeout issue on iOS for fetchProducts by replacing the blocking DispatchSemaphore with a modern asynchronous Task-based approach that emits a signal. While this improves thread safety and responsiveness, a security concern was identified regarding the logging of sensitive information in the GDScript wrapper due to signal pollution. Additionally, this introduces a breaking API change and appears to break existing unit tests which call fetch_products synchronously. Please review the detailed comments for specific feedback.
| func fetch_products(request: Types.ProductRequest) -> Array: | ||
| print("[GodotIap] fetch_products called") | ||
| var result = _fetch_products_raw(request.to_dict()) | ||
| var result = await _fetch_products_raw(request.to_dict()) |
There was a problem hiding this comment.
This await makes fetch_products and _fetch_products_raw asynchronous. This breaking change appears to break the unit tests in Example/tests/test_godot_iap.gd. The test test_fetch_products_mock calls fetch_products synchronously, which will now cause the test to fail because it receives a GDScriptFunctionState object instead of an Array. The tests need to be updated to use await when calling fetch_products to work with this change.
There was a problem hiding this comment.
Fixed in 9005c03. Updated the test to use await for fetch_products.
| _native_plugin.fetchProducts(request_json) | ||
| # Await the signal from Swift native plugin | ||
| var signal_result: Dictionary = await products_fetched | ||
| print("[GodotIap] fetchProducts signal result: ", signal_result) |
There was a problem hiding this comment.
The code logs the result of the products_fetched signal. However, this signal is shared across many different operations in the native iOS plugin, including sensitive ones like verifyPurchase, getReceiptDataIOS, and getTransactionJwsIOS. If any of these sensitive operations complete while fetch_products is awaiting the signal, their results (which can contain sensitive data like receipt data, JWS tokens, or purchase tokens) will be captured and printed to the application logs. It is recommended to remove this print statement or sanitize the output to prevent leaking sensitive IAP data in production logs.
There was a problem hiding this comment.
Good catch. Removed the signal result print statement in 9005c03 to prevent sensitive data exposure.
| ## Fetch products from the store | ||
| ## @param request: Types.ProductRequest object | ||
| ## Returns Array of typed product objects (Types.ProductAndroid or Types.ProductIOS) | ||
| ## Note: On iOS this awaits the products_fetched signal internally |
There was a problem hiding this comment.
The docstring could be clearer about the implications of this change. Since fetch_products now uses await, it has become an asynchronous function and must be awaited by any caller. This is a significant breaking change to the API. To avoid confusion for developers using this plugin, I recommend making the asynchronous nature more explicit in the documentation.
For example:
## Note: This function is asynchronous and must be called with 'await'.
## On iOS, it awaits the 'products_fetched' signal internally.There was a problem hiding this comment.
Fixed in 9005c03. Updated docstring to explicitly state that the function is async and must be called with await.
- Improve docstring to clarify async/await requirement - Remove signal result logging to prevent sensitive data exposure - Update test to use await for fetch_products Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
DispatchSemaphoreblocking pattern in iOSfetchProducts()that caused thread starvation deadlocksTask+ signal emission pattern (matchingrequestPurchaseand other iOS methods)await products_fetchedsignal on iOSRoot Cause
The iOS
fetchProducts()used aDispatchSemaphoreto bridge async StoreKit 2 calls to synchronous GDExtension returns. This blocked Godot's main thread, preventing the asyncTaskfrom executing, causing a deadlock that always resulted in a 30-second timeout.Changes
iOS (
ios-gdextension/)DispatchSemaphoreand 30-second timeout logic{"status": "pending"}immediately (likerequestPurchase)products_fetchedsignal asynchronouslyGDScript (
addons/godot-iap/)fetchProducts()thenawait products_fetchedsignalTest plan
fetch_products()returns products without timeoutfetch_products()continues to work as beforeCloses #15
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests
Documentation