Skip to content

Commit a635cf0

Browse files
CopilotbedaHovorka
authored andcommitted
Fix non-unique Koin scope ids in DefaultSimulationContext/DefaultEditingContext
1 parent 3d03f59 commit a635cf0

5 files changed

Lines changed: 109 additions & 5 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/* Brno University of Technology
2+
* Faculty of Information Technology
3+
*
4+
* BSc Thesis 2006/2007
5+
*
6+
* Railway Interlocking Simulator
7+
*
8+
* Bedrich Hovorka
9+
*/
10+
package cz.vutbr.fit.interlockSim.context
11+
12+
import kotlinx.atomicfu.atomic
13+
14+
/**
15+
* Generates unique Koin scope ids for [Context] implementations.
16+
*
17+
* [DefaultEditingContext] and [DefaultSimulationContext] each need a scope id that is
18+
* guaranteed unique for the lifetime of the process. Identity hash codes (as used by
19+
* [Any.hashCode] on objects that don't override it, or [cz.vutbr.fit.interlockSim.util.platformIdentityCode])
20+
* are **not** unique - they live in a 31-bit space and collisions become likely once tens of
21+
* thousands of contexts have been created (birthday bound), which throws
22+
* `ScopeAlreadyCreatedException` (see Issue #757). A monotonic counter guarantees uniqueness
23+
* regardless of how many contexts are created.
24+
*/
25+
private val nextContextScopeId = atomic(0L)
26+
27+
/**
28+
* Returns a new, process-wide unique id suitable for use as a Koin `scopeId`.
29+
*/
30+
internal fun nextContextScopeId(): String = nextContextScopeId.incrementAndGet().toString()

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/context/DefaultEditingContext.kt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,10 @@ open class DefaultEditingContext(
9292
.defaultContext()
9393
.get()
9494
.createScope(
95-
// DefaultEditingContext does not override hashCode(), so this.hashCode()
96-
// returns the identity hash code (equivalent to System.identityHashCode(this)).
97-
// Each context instance gets a unique scope ID.
98-
scopeId = this.hashCode().toString(),
95+
// Uses a monotonic counter rather than an identity hash code, which is not
96+
// guaranteed unique and can collide under high context creation volume
97+
// (Issue #757).
98+
scopeId = nextContextScopeId(),
9999
qualifier =
100100
org.koin.core.qualifier
101101
.named<DefaultEditingContext>(),

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/context/DefaultSimulationContext.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ open class DefaultSimulationContext(
150150
.defaultContext()
151151
.get()
152152
.createScope(
153-
scopeId = platformIdentityCode(this),
153+
scopeId = nextContextScopeId(),
154154
qualifier =
155155
org.koin.core.qualifier
156156
.named<DefaultSimulationContext>(),

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/util/PlatformIdentity.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,9 @@ package cz.vutbr.fit.interlockSim.util
88
*
99
* - **JVM:** delegates to [System.identityHashCode]
1010
* - **Native:** delegates to [kotlin.native.identityHashCode]
11+
*
12+
* **Do not use this as a unique identifier** (e.g. as a map/scope key). Identity hash codes live
13+
* in a 31-bit space, so collisions become likely at moderate object counts (birthday bound) -
14+
* this is for human-readable debug logging only. See Issue #757.
1115
*/
1216
expect fun platformIdentityCode(obj: Any): String
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/* Brno University of Technology
2+
* Faculty of Information Technology
3+
*
4+
* BSc Thesis 2006/2007
5+
*
6+
* Railway Interlocking Simulator
7+
*
8+
* Bedrich Hovorka
9+
*/
10+
package cz.vutbr.fit.interlockSim.context
11+
12+
import assertk.assertThat
13+
import assertk.assertions.hasSize
14+
import cz.vutbr.fit.interlockSim.testutil.commonCoreTestModule
15+
import org.koin.core.context.startKoin
16+
import org.koin.core.context.stopKoin
17+
import kotlin.test.AfterTest
18+
import kotlin.test.BeforeTest
19+
import kotlin.test.Test
20+
21+
/**
22+
* Regression tests for Issue #757: [DefaultEditingContext] and [DefaultSimulationContext]
23+
* used to derive their Koin `scopeId` from an identity hash code (`this.hashCode()` /
24+
* [cz.vutbr.fit.interlockSim.util.platformIdentityCode]). Identity hash codes live in a
25+
* 31-bit space, so under high context-creation volume (tens of thousands of contexts, e.g.
26+
* JMH `AverageTime` benchmarks) two live contexts could collide and Koin would throw
27+
* `ScopeAlreadyCreatedException`.
28+
*
29+
* The fix replaces the identity hash code with a process-wide monotonic counter
30+
* ([nextContextScopeId]), which is unique by construction regardless of how many contexts
31+
* are created concurrently.
32+
*/
33+
class ContextScopeIdUniquenessTest {
34+
@BeforeTest
35+
fun setUpKoin() {
36+
startKoin { modules(commonCoreTestModule) }
37+
}
38+
39+
@AfterTest
40+
fun tearDownKoin() {
41+
stopKoin()
42+
}
43+
44+
@Test
45+
fun scopeIdsAreUniqueAcrossManyEditingContexts() {
46+
// Enough instances that identity-hash-code collisions (31-bit space, birthday bound)
47+
// would have been likely with the old implementation, without making the test slow.
48+
val count = 5_000
49+
val scopeIds = mutableSetOf<String>()
50+
val contexts = mutableListOf<DefaultEditingContext>()
51+
try {
52+
repeat(count) {
53+
val context = DefaultEditingContext(2, 2)
54+
contexts.add(context)
55+
scopeIds.add(context.scope.id)
56+
}
57+
// Every context must have received a distinct scope id: no collisions, and thus
58+
// no ScopeAlreadyCreatedException would have been thrown while creating them.
59+
assertThat(scopeIds).hasSize(count)
60+
} finally {
61+
contexts.forEach { it.close() }
62+
}
63+
}
64+
65+
@Test
66+
fun nextContextScopeIdIsMonotonicAndUnique() {
67+
val ids = (1..1_000).map { nextContextScopeId() }
68+
assertThat(ids.toSet()).hasSize(ids.size)
69+
}
70+
}

0 commit comments

Comments
 (0)