Fix Scope Factory Sync Issue (#2233)#2234
Conversation
|
@arnaudgiuliani Could you please help, when is it planned to merge and release this fix? Thanks in advance! |
|
Any test to help would be appreciated 🙏 |
I do not know how to reproduce this error reliably. It occurs suddenly from time to time during operation in a production environment. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #2233 regarding thread-insecure collection usage in ScopedInstanceFactory. The changes replace the regular HashMap with a platform-specific thread-safe map (ConcurrentHashMap on JVM, ConcurrentMutableMap on other platforms) and remove the synchronized block that was protecting instance creation.
Key changes:
- Changed
valuesfromvarwithhashMapOf()tovalwithKoinPlatformTools.safeHashMap() - Removed
synchronizedblock from theget()method - Removed blank lines for formatting consistency
However, the implementation introduces a race condition by removing synchronization without using atomic operations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| values[context.scope.id] = super.create(context) | ||
| } | ||
| if (!isCreated(context) && holdInstance) { | ||
| values[context.scope.id] = super.create(context) |
There was a problem hiding this comment.
Race condition: The check-then-act pattern here is not thread-safe. Multiple threads can pass the !isCreated(context) check simultaneously, causing super.create(context) to be invoked multiple times for the same scope ID. This violates the singleton-per-scope contract.
While safeHashMap provides a thread-safe map, it doesn't prevent this race condition. Consider using computeIfAbsent (on JVM's ConcurrentHashMap) or restoring the synchronized block to ensure atomic check-and-create operations. See SingleInstanceFactory.get() for a similar pattern that uses synchronization correctly.
| values[context.scope.id] = super.create(context) | |
| KoinPlatformTools.synchronized(this) { | |
| if (!isCreated(context)) { | |
| values[context.scope.id] = super.create(context) | |
| } | |
| } |
|
Rewoked in #2323 |
fix Thread-insecure collection without synchronized in ScopedInstanceFactory #2233