Fix critical bug for multiple stores listening to same action, add tests for it

Author: Pablo Orgaz

build.gradle

@@ -11,7 +11,7 @@ buildscript {
     }
 
     dependencies {
-        classpath 'com.android.tools.build:gradle:3.3.1'
+        classpath 'com.android.tools.build:gradle:3.3.2'
         classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
         classpath 'org.junit.platform:junit-platform-gradle-plugin:1.0.0'
         classpath 'com.github.triplet.gradle:play-publisher:1.2.0'

mini-common/build.gradle

@@ -18,6 +18,7 @@ dependencies {
     compile "io.reactivex.rxjava2:rxjava:$rx_version"
 
     testCompile 'junit:junit:4.12'
+    testImplementation "org.amshove.kluent:kluent:1.44"
 }
 
 apply plugin: 'idea'

mini-common/src/main/java/mini/Dispatcher.kt

@@ -2,6 +2,7 @@ package mini
 
 import io.reactivex.disposables.Disposable
 import java.util.*
+import java.util.concurrent.atomic.AtomicInteger
 import kotlin.reflect.KClass
 
 /**
@@ -66,7 +67,12 @@ class Dispatcher {
         synchronized(subscriptions) {
             val reg = Registration(this, clazz.java, priority, callback as (Any) -> Unit)
             val set = subscriptions.getOrPut(clazz.java) {
-                TreeSet(kotlin.Comparator { a, b -> a.priority.compareTo(b.priority) })
+                TreeSet(kotlin.Comparator { a, b ->
+                    //Sort by priority, then by id for equal priority
+                    val p = a.priority.compareTo(b.priority)
+                    if (p == 0) a.id.compareTo(b.id)
+                    else p
+                })
             }
             set.add(reg)
             return reg
@@ -85,15 +91,22 @@ class Dispatcher {
      * the main thread.
      */
     fun dispatch(action: Any) {
-        onUiSync {
-            if (dispatching != null) {
-                throw IllegalStateException("Nested dispatch calls. Currently dispatching: " +
-                                            "$dispatching. Nested action: $action ")
-            }
-            dispatching = action
-            interceptorChain.proceed(action)
-            dispatching = null
+        if (isAndroid) {
+            onUiSync { doDispatch(action) }
+        } else {
+            doDispatch(action)
+        }
+    }
+
+    @Suppress("NOTHING_TO_INLINE") //Inlined so it doesn't appear in stack trace
+    private inline fun doDispatch(action: Any) {
+        if (dispatching != null) {
+            throw IllegalStateException("Nested dispatch calls. Currently dispatching: " +
+                                        "$dispatching. Nested action: $action ")
         }
+        dispatching = action
+        interceptorChain.proceed(action)
+        dispatching = null
     }
 
     /**
@@ -104,15 +117,39 @@ class Dispatcher {
         onUi { dispatch(action) }
     }
 
-    data class Registration(val dispatcher: Dispatcher,
-                            val type: Class<*>,
-                            val priority: Int, val fn: (Any) -> Unit) : Disposable {
+    /**
+     * Handle for a dispatcher subscription.
+     */
+    class Registration(val dispatcher: Dispatcher,
+                       val type: Class<*>,
+                       val priority: Int, val fn: (Any) -> Unit) : Disposable {
+
+        companion object {
+            private val idCounter = AtomicInteger()
+        }
+
+        val id = idCounter.getAndIncrement()
         var disposed = false
+
+        @Synchronized
         override fun isDisposed(): Boolean = disposed
 
+        @Synchronized
         override fun dispose() {
             dispatcher.unregister(this)
             disposed = true
         }
+
+        override fun equals(other: Any?): Boolean {
+            if (this === other) return true
+            if (javaClass != other?.javaClass) return false
+            other as Registration
+            if (id != other.id) return false
+            return true
+        }
+
+        override fun hashCode(): Int {
+            return id
+        }
     }
 }

mini-common/src/test/kotlin/mini/DispatcherTest.kt

@@ -0,0 +1,61 @@
+package mini
+
+import org.amshove.kluent.`should be equal to`
+import org.junit.Test
+
+class DispatcherTest {
+
+    class TestAction : BaseAction()
+
+    @Test
+    fun subscriptions_are_added() {
+        val dispatcher = Dispatcher()
+        var called = 0
+        dispatcher.register<TestAction> {
+            called++
+        }
+        dispatcher.dispatch(TestAction())
+        called `should be equal to` 1
+    }
+
+    @Test
+    fun order_is_respected_for_same_priority() {
+        val dispatcher = Dispatcher()
+        val calls = ArrayList<Int>()
+        dispatcher.register<TestAction> {
+            calls.add(0)
+        }
+        dispatcher.register<TestAction> {
+            calls.add(1)
+        }
+        dispatcher.dispatch(TestAction())
+        calls[0] `should be equal to` 0
+        calls[1] `should be equal to` 1
+    }
+
+    @Test
+    fun order_is_respected_for_different_priority() {
+        val dispatcher = Dispatcher()
+        val calls = ArrayList<Int>()
+        dispatcher.register<TestAction>(priority = 10) {
+            calls.add(0)
+        }
+        dispatcher.register<TestAction>(priority = 0) {
+            calls.add(1)
+        }
+        dispatcher.dispatch(TestAction())
+        calls[0] `should be equal to` 1
+        calls[1] `should be equal to` 0
+    }
+
+    @Test
+    fun disposing_registration_removes_subscription() {
+        val dispatcher = Dispatcher()
+        var called = 0
+        dispatcher.register<TestAction> {
+            called++
+        }.dispose()
+        dispatcher.dispatch(TestAction())
+        called `should be equal to` 0
+    }
+}

mini-runtime/build.gradle

@@ -9,5 +9,6 @@ dependencies {
     compile 'io.github.classgraph:classgraph:4.6.32'
 
     testCompile 'junit:junit:4.12'
-    testCompile 'com.google.testing.compile:compile-testing:0.15'
+    testCompile "org.amshove.kluent:kluent:1.44"
+
 }

mini-runtime/src/main/java/mini/MiniRuntime.kt

@@ -3,6 +3,7 @@ package mini
 import kotlin.reflect.KClass
 import kotlin.reflect.full.findAnnotation
 import kotlin.reflect.full.functions
+import kotlin.reflect.jvm.javaType
 import kotlin.reflect.jvm.jvmErasure
 
 object MiniRuntime : MiniInitializer {
@@ -16,6 +17,11 @@ object MiniRuntime : MiniInitializer {
                     if (fn.parameters.size != 2) { //param 0 is `this`
                         throw IllegalArgumentException("Function should have 1 argument for action")
                     }
+
+                    if (fn.returnType.javaType != Void.TYPE) {
+                        throw IllegalArgumentException("Reducers shouldn't have return value")
+                    }
+
                     val actionType = fn.parameters[1].type.jvmErasure
                     val priority = annotation!!.priority
                     dispatcher.register(clazz = actionType, priority = priority, callback = {
@@ -43,11 +49,9 @@ object MiniRuntime : MiniInitializer {
     private class ReflectedType(val clazz: KClass<*>, val depth: Int)
 
     class ReflectiveActionTypesMap : Map<Class<*>, List<Class<*>>> {
-
         private val genericTypes = listOf(
             Object::class.java
         )
-
         private val map = HashMap<Class<*>, List<Class<*>>>()
         override val entries: Set<Map.Entry<Class<*>, List<Class<*>>>> = map.entries
         override val keys: Set<Class<*>> = map.keys

mini-runtime/src/test/kotlin/mini/MiniRuntimeTest.kt

@@ -0,0 +1,56 @@
+package mini
+
+import org.amshove.kluent.`should be equal to`
+import org.amshove.kluent.`should contain`
+import org.junit.Test
+
+class MiniRuntimeTest {
+
+    class SampleAction : BaseAction()
+
+    class ValidStore : Store<String>() {
+        @Reducer
+        fun reducer(action: SampleAction) {
+        }
+    }
+
+    class InvalidStore1 : Store<String>() {
+        @Reducer
+        fun reducer(action: SampleAction): String {
+            return "Invalid return"
+        }
+    }
+
+    class InvalidStore2 : Store<String>() {
+        @Reducer
+        fun reducer(action: SampleAction, badParam: String): String {
+            return "Invalid return"
+        }
+    }
+
+    @Test
+    fun types_are_reflected() {
+        val map = MiniRuntime.ReflectiveActionTypesMap()
+        map[SampleAction::class.java]!!.`should contain`(SampleAction::class.java)
+        map[SampleAction::class.java]!!.`should contain`(BaseAction::class.java)
+    }
+
+    @Test
+    fun reducers_are_found() {
+        val dispatcher = Dispatcher()
+        MiniRuntime.initialize(dispatcher, listOf(ValidStore()))
+        dispatcher.subscriptions.size `should be equal to` 1
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun functions_should_be_void() {
+        val dispatcher = Dispatcher()
+        MiniRuntime.initialize(dispatcher, listOf(InvalidStore1()))
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun functions_should_have_one_parameter_void() {
+        val dispatcher = Dispatcher()
+        MiniRuntime.initialize(dispatcher, listOf(InvalidStore2()))
+    }
+}