fix(godot): add initialization guard to prevent double plugin init#87
Conversation
When the plugin is both enabled in ProjectSettings and attached as an AutoLoad node, _ready() runs twice causing duplicate initialization, listeners, and state conflicts. - Add _is_initialized guard in _ready() to prevent double native plugin init - Add _is_connected guard in init_connection() to make it idempotent Closes hyochan/godot-iap#24 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 introduces guards to prevent redundant initialization and connection logic in godot_iap.gd. Specifically, it adds an _is_initialized flag in _ready() and an _is_connected check in init_connection(). Feedback suggests that the instance-level initialization guard may not suffice if multiple script instances are being created, recommending a check for existing signal connections on the native plugin as a more robust solution.
| if _is_initialized: | ||
| return | ||
| _is_initialized = true |
There was a problem hiding this comment.
The use of an instance variable _is_initialized for this guard only prevents multiple calls to _ready() on the same instance (e.g., if a node is removed and re-added to the tree).
If the issue described in the PR (duplicate initialization from both Plugin and AutoLoad) results in two separate instances of the script being created, this guard will not prevent both from initializing the native plugin and connecting duplicate signals. While this is a good improvement for robustness, a more definitive fix for duplicate listeners would be to check _native_plugin.is_connected(signal_name, method) before calling connect() in the _connect_signals_* methods.
There was a problem hiding this comment.
Fixed in 468d476. Changed _is_initialized to static var so it guards across all instances.
📝 WalkthroughWalkthroughAdded an initialization guard Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libraries/godot-iap/addons/godot-iap/godot_iap.gd`:
- Around line 30-38: The instance-level _is_initialized allows each
GodotIapWrapper to run initialization separately; change it to a
class-level/static guard (e.g., declare _is_initialized as a static var) and use
that static flag inside _ready() to short-circuit duplicate initialization and
only call _init_native_plugin() when the static flag is false, then set the
static flag true; update references to _is_initialized and ensure the check in
_ready() and any calls to _init_native_plugin() use the new static/class-level
variable so native plugin init runs only once across all instances.
- Around line 184-186: The early-return in init_connection() that returns true
when _is_connected is set causes false-positive success (especially on iOS where
native initConnection() is async) so remove that cached shortcut: always
probe/validate the native connection state instead of returning based on
_is_connected alone (or remove the early-return branch entirely) so
init_connection() will call the native initConnection() flow and wait for its
actual async result before signaling success; update checks around the native
initConnection() call and the _is_connected setter to reflect the real native
callback/state transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2464ea39-8582-42e3-8949-5431a807360f
📒 Files selected for processing (1)
libraries/godot-iap/addons/godot-iap/godot_iap.gd
…tion - Test _is_initialized guard prevents double _ready() execution - Test init_connection is idempotent when already connected Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address review feedback: - Make _is_initialized a static var so it guards across all instances - Remove _is_connected early return in init_connection() to avoid false-positive success on iOS async connections Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libraries/godot-iap/Example/tests/test_godot_iap.gd (1)
64-71: Idempotency test validates return value, not short-circuit behavior.Line 71 confirms
trueon the second call, but that can still pass even ifinitConnectionis invoked again under the hood. Consider using a mock native plugin with a call counter and assert the second call does not increment it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libraries/godot-iap/Example/tests/test_godot_iap.gd` around lines 64 - 71, The test test_init_connection_idempotent currently only checks return values and not that GodotIapPlugin.init_connection short-circuits via the _is_connected guard; update the test to inject or swap in a mock native plugin (a fake that exposes a call counter) before calling GodotIapPlugin.init_connection, call it twice, and assert the native plugin's init call counter increments only once (i.e., second call does not invoke the underlying initConnection); reference the test function test_init_connection_idempotent, the class GodotIapPlugin, and the internal guard _is_connected (or native initConnection) when locating where to inject the mock and perform the counter assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libraries/godot-iap/Example/tests/test_godot_iap.gd`:
- Around line 53-61: The test test_ready_guard_prevents_double_init currently
only compares GodotIapPlugin._platform which can be unchanged even if
re-initialization ran; change the test to assert a concrete, non-idempotent
side-effect did not occur a second time: e.g., record a "native init" invocation
count or signal connection count before the second GodotIapPlugin._ready() call
(add or use a spy like GodotIapPlugin._native_init_call_count or inspect the
number of connections for a known signal), call GodotIapPlugin._ready() again,
and assert that the invocation/connection count did not increase while still
asserting GodotIapPlugin._is_initialized is true. Ensure you reference and
update the test to check that the native init was not invoked again rather than
relying on _platform.
- Around line 26-29: The tests leak shared plugin state: running
test_init_connection_idempotent() can leave GodotIapPlugin._is_connected true
and cause test_init_connection_mock() to only hit the short-circuit path; reset
or isolate state between tests by clearing GodotIapPlugin._is_connected (or
calling the plugin disconnect/cleanup helper) before/after each test, or reorder
tests so test_init_connection_mock() runs before
test_init_connection_idempotent(); update the test file around
test_ready_guard_prevents_double_init, test_init_connection_idempotent, and
test_init_connection_mock to ensure each test explicitly resets
GodotIapPlugin._is_connected (or uses a setup/teardown) so mock initialization
is exercised independently.
---
Nitpick comments:
In `@libraries/godot-iap/Example/tests/test_godot_iap.gd`:
- Around line 64-71: The test test_init_connection_idempotent currently only
checks return values and not that GodotIapPlugin.init_connection short-circuits
via the _is_connected guard; update the test to inject or swap in a mock native
plugin (a fake that exposes a call counter) before calling
GodotIapPlugin.init_connection, call it twice, and assert the native plugin's
init call counter increments only once (i.e., second call does not invoke the
underlying initConnection); reference the test function
test_init_connection_idempotent, the class GodotIapPlugin, and the internal
guard _is_connected (or native initConnection) when locating where to inject the
mock and perform the counter assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55a58f0d-2b29-4ad4-a671-4f12aeadc6ac
📒 Files selected for processing (1)
libraries/godot-iap/Example/tests/test_godot_iap.gd
- Reorder tests: connection tests run before guard tests to avoid state leakage - Verify _ready() guard by checking signal connection count doesn't increase - Reset _is_connected before idempotent test for isolation - Add test_no_duplicate_signal_connections for PR test plan coverage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libraries/godot-iap/addons/godot-iap/godot_iap.gd`:
- Around line 36-38: The static guard _is_initialized causes later
GodotIapWrapper instances to skip per-instance setup leaving _native_plugin
null; change the initialization logic in the constructor/initializer so that
while shared/global initialization guarded by _is_initialized still runs only
once, per-instance fields like _native_plugin are always set (e.g., call the
native plugin resolution and assign _native_plugin and any other
instance-specific state even when _is_initialized is true) so methods such as
init_connection on subsequent GodotIapWrapper instances don't fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c94f7371-5ba1-4d54-9687-ba2b69c964f2
📒 Files selected for processing (2)
libraries/godot-iap/Example/tests/test_godot_iap.gdlibraries/godot-iap/addons/godot-iap/godot_iap.gd
✅ Files skipped from review due to trivial changes (1)
- libraries/godot-iap/Example/tests/test_godot_iap.gd
| if _is_initialized: | ||
| return | ||
| _is_initialized = true |
There was a problem hiding this comment.
Static guard currently leaves later GodotIapWrapper instances uninitialized.
Early return on _is_initialized skips per-instance state setup. A second wrapper instance can keep _native_plugin == null, so calls on that instance (e.g., init_connection) fail even though initialization already happened elsewhere.
Proposed fix
static var _is_initialized: bool = false
+static var _shared_native_plugin: Object = null
# Platform detection
var _platform: String = ""
func _ready() -> void:
- if _is_initialized:
- return
- _is_initialized = true
_platform = OS.get_name()
+
+ if _is_initialized:
+ # Reuse class-level initialized native handle for this instance.
+ _native_plugin = _shared_native_plugin
+ return
+
_init_native_plugin()
+ _shared_native_plugin = _native_plugin
+ _is_initialized = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libraries/godot-iap/addons/godot-iap/godot_iap.gd` around lines 36 - 38, The
static guard _is_initialized causes later GodotIapWrapper instances to skip
per-instance setup leaving _native_plugin null; change the initialization logic
in the constructor/initializer so that while shared/global initialization
guarded by _is_initialized still runs only once, per-instance fields like
_native_plugin are always set (e.g., call the native plugin resolution and
assign _native_plugin and any other instance-specific state even when
_is_initialized is true) so methods such as init_connection on subsequent
GodotIapWrapper instances don't fail.
1 participant
Summary
static var _is_initializedguard in_ready()to prevent double native plugin init across all instancesinit_connection()to avoid false-positive success on iOS async connectionsWhen the plugin is both enabled in ProjectSettings > Plugins AND the
godot_iap.gdscript is attached as an AutoLoad node,_ready()runs twice causing duplicate initialization, listeners, and state conflicts.Follows up from hyochan/godot-iap#25 / hyochan/godot-iap#24
Test plan
test_no_duplicate_signal_connections🤖 Generated with Claude Code