[core] Ensure onClose callbacks are invoked in reversed initialization order

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/extension/KoinExtension.kt

@@ -25,7 +25,7 @@ import org.koin.core.Koin
 interface KoinExtension {
 
     /**
-     * register from Koin instance
+     * Register from Koin instance
      */
     fun onRegister(koin : Koin)
 

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/instance/InstanceFactory.kt

@@ -17,6 +17,8 @@
 
 package org.koin.core.instance
 
+import co.touchlab.stately.concurrency.AtomicBoolean
+import co.touchlab.stately.concurrency.AtomicLong
 import org.koin.core.definition.BeanDefinition
 import org.koin.core.error.InstanceCreationException
 import org.koin.core.parameter.ParametersHolder
@@ -28,8 +30,12 @@ import org.koin.mp.Lockable
 /**
  * Koin Instance Holder
  * create/get/release an instance of given definition
+ *
+ * Implements [Comparable] to provide factories soring by instance creation order
  */
-abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Lockable() {
+abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Lockable(), Comparable<InstanceFactory<*>> {
+    private val instanceCreationOrderSet = AtomicBoolean(false)
+    private var instanceCreationOrderPosition = Long.MAX_VALUE
 
     /**
      * Retrieve an instance
@@ -47,17 +53,36 @@ abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Locka
         context.logger.debug("| (+) '$beanDefinition'")
         try {
             val parameters: ParametersHolder = context.parameters ?: emptyParametersHolder()
-            return beanDefinition.definition.invoke(
+            val instance = beanDefinition.definition.invoke(
                 context.scope,
                 parameters,
             )
+            if (instanceCreationOrderSet.compareAndSet(false, true)) {
+                instanceCreationOrderPosition = INSTANCE_CREATION_ORDER_COUNTER.incrementAndGet()
+
+                // just to be sure that counter provides normal values
+                check(instanceCreationOrderPosition >= 0) {
+                    "Unexpected negative instance creation order position"
+                }
+
+                // it's required to have `Long.MAX_VALUE` instantiations happened to make this check fail,
+                // which is not likely to happen
+                check(instanceCreationOrderPosition < Long.MAX_VALUE) {
+                    "Instance creation order position reached Long.MAX_VALUE"
+                }
+            }
+            return instance
         } catch (e: Exception) {
             val stack = KoinPlatformTools.getStackTrace(e)
             context.logger.error("* Instance creation error : could not create instance for '$beanDefinition': $stack")
             throw InstanceCreationException("Could not create instance for '$beanDefinition'", e)
         }
     }
 
+    override fun compareTo(other: InstanceFactory<*>): Int {
+        return instanceCreationOrderPosition.compareTo(other.instanceCreationOrderPosition)
+    }
+
     /**
      * Is instance created
      */
@@ -72,5 +97,6 @@ abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Locka
 
     companion object {
         const val ERROR_SEPARATOR = "\n\t"
+        private val INSTANCE_CREATION_ORDER_COUNTER = AtomicLong(-1)
     }
 }

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/registry/InstanceRegistry.kt

@@ -32,6 +32,8 @@ import org.koin.core.scope.Scope
 import org.koin.core.scope.ScopeID
 import org.koin.mp.KoinPlatformTools.safeHashMap
 import kotlin.reflect.KClass
+import kotlin.sequences.sortedDescending
+
 
 @Suppress("UNCHECKED_CAST")
 @OptIn(KoinInternalApi::class)
@@ -152,13 +154,18 @@ class InstanceRegistry(val _koin: Koin) {
     }
 
     internal fun dropScopeInstances(scope: Scope) {
-        _instances.values.filterIsInstance<ScopedInstanceFactory<*>>().forEach { factory -> factory.drop(scope) }
+        _instances.values
+            .asSequence()
+            .filterIsInstance<ScopedInstanceFactory<*>>()
+            .sortedDescending()
+            .forEach { factory -> factory.drop(scope) }
     }
 
     internal fun close() {
-        _instances.forEach { (_, factory) ->
-            factory.dropAll()
-        }
+        _instances.values
+            .asSequence()
+            .sortedDescending()
+            .forEach { factory -> factory.dropAll() }
         _instances.clear()
     }
 
@@ -181,12 +188,10 @@ class InstanceRegistry(val _koin: Koin) {
     }
 
     private fun unloadModule(module: Module) {
-        module.mappings.keys.forEach { mapping ->
-            if (_instances.containsKey(mapping)) {
-                _instances[mapping]?.dropAll()
-                _instances.remove(mapping)
-            }
-        }
+        module.mappings.keys
+            .mapNotNull { mapping -> _instances.remove(mapping) }
+            .sortedDescending()
+            .forEach { factory -> factory.dropAll() }
     }
 
     fun size(): Int {

projects/core/koin-core/src/commonTest/kotlin/org/koin/dsl/CloseDefinitionTest.kt

@@ -1,8 +1,12 @@
 package org.koin.dsl
 
 import org.koin.Simple
+import org.koin.core.annotation.KoinInternalApi
 import org.koin.core.logger.Level
+import org.koin.core.qualifier.named
 import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
 
 class CloseDefinitionTest {
@@ -50,4 +54,186 @@ class CloseDefinitionTest {
         koin.unloadModules(listOf(module))
         assertTrue(!closed)
     }
+
+    @OptIn(KoinInternalApi::class)
+    @Test
+    fun test_onClose_order() {
+        val closeLog = mutableListOf<String>()
+
+        val module = module {
+            single { Simple.ComponentC(get()) } onClose {
+                if (it != null) {
+                    closeLog += "c"
+                }
+            }
+            single { Simple.ComponentA() } onClose {
+                if (it != null) {
+                    closeLog += "a"
+                }
+            }
+            single { Simple.ComponentB(get()) } onClose {
+                if (it != null) {
+                    closeLog += "b"
+                }
+            }
+        }
+
+        val koin = koinApplication {
+            printLogger(Level.DEBUG)
+            modules(
+                module,
+            )
+        }.koin
+
+        assertNotNull(koin.getOrNull<Simple.ComponentC>())
+        assertEquals(
+            expected = emptyList(), 
+            actual = closeLog,
+        )
+
+        // check instances sorting meets instantiation order
+        assertEquals(
+            expected = listOf(Simple.ComponentA::class, Simple.ComponentB::class, Simple.ComponentC::class),
+            actual = koin.instanceRegistry.instances.values.sorted().map { it.beanDefinition.primaryType },
+        )
+
+        koin.close()
+        assertEquals(
+            expected = listOf("c", "b", "a"), 
+            actual = closeLog,
+        )
+    }
+
+    @Test
+    fun test_onClose_order_unload() {
+        val closeLog = mutableListOf<String>()
+
+        val moduleFirst = module {
+            single { Simple.MySingle(42) } onClose {
+                if (it != null) {
+                    closeLog += "single"
+                }
+            }
+        }
+
+        val moduleSecond = module {
+            single { Simple.ComponentC(get()) } onClose {
+                if (it != null) {
+                    closeLog += "c"
+                }
+            }
+            single { Simple.ComponentA() } onClose {
+                if (it != null) {
+                    closeLog += "a"
+                }
+            }
+            single { Simple.ComponentB(get()) } onClose {
+                if (it != null) {
+                    closeLog += "b"
+                }
+            }
+        }
+
+        val koin = koinApplication {
+            printLogger(Level.DEBUG)
+            modules(
+                moduleFirst,
+                moduleSecond,
+            )
+        }.koin
+
+        assertNotNull(koin.getOrNull<Simple.MySingle>())
+        assertNotNull(koin.getOrNull<Simple.ComponentC>())
+        assertEquals(emptyList(), closeLog)
+
+        koin.unloadModules(listOf(moduleSecond))
+        assertEquals(
+            expected = listOf("c", "b", "a"), 
+            actual = closeLog,
+        )
+
+        koin.loadModules(listOf(moduleSecond))
+        assertNotNull(koin.getOrNull<Simple.ComponentC>())
+
+        koin.unloadModules(listOf(moduleSecond))
+        assertEquals(
+            expected = listOf("c", "b", "a", "c", "b", "a"),
+            actual = closeLog,
+        )
+
+        koin.close()
+        assertEquals(
+            expected = listOf("c", "b", "a", "c", "b", "a", "single"),
+            actual = closeLog,
+        )
+    }
+
+
+    @Test
+    fun test_onClose_order_scoped() {
+        val closeLog = mutableListOf<String>()
+
+        val scopeId = "_test_scope_id_"
+        val scopeKey = named("_test_scope_")
+
+        val module = module {
+            scope(scopeKey) {
+                scoped { Simple.ComponentC(get()) } onClose {
+                    if (it != null) {
+                        closeLog += "c"
+                    }
+                }
+                scoped { Simple.ComponentA() } onClose {
+                    if (it != null) {
+                        closeLog += "a"
+                    }
+                }
+                scoped { Simple.ComponentB(get()) } onClose {
+                    if (it != null) {
+                        closeLog += "b"
+                    }
+                }
+            }
+        }
+
+        val koin = koinApplication {
+            printLogger(Level.DEBUG)
+            modules(
+                module,
+            )
+        }.koin
+
+        assertEquals(
+            expected = emptyList(),
+            actual = closeLog,
+        )
+
+        koin.createScope(scopeId, scopeKey)
+        assertEquals(
+            expected = emptyList(),
+            actual = closeLog,
+        )
+
+        koin.getScope(scopeId).let {
+            assertNotNull(it.getOrNull<Simple.ComponentC>())
+            it.close()
+        }
+        assertEquals(
+            expected = listOf("c", "b", "a"),
+            actual = closeLog,
+        )
+
+        koin.createScope(scopeId, scopeKey)
+        assertNotNull(koin.getScope(scopeId).getOrNull<Simple.ComponentC>())
+        assertEquals(
+            expected = listOf("c", "b", "a"),
+            actual = closeLog,
+        )
+
+        koin.close()
+        assertEquals(
+            expected = listOf("c", "b", "a", "c", "b", "a"),
+            actual = closeLog,
+        )
+    }
 }