fix race condition that causes plugin update with incorrect type (#196)

wenxi-zeng · web-flow · commit 8f720da419fd · 2023-11-03T13:34:08.000-07:00
* add initializedPlugins to system state

* fix broken tests

* add unit tests
diff --git a/android/src/test/java/com/segment/analytics/kotlin/android/StorageTests.kt b/android/src/test/java/com/segment/analytics/kotlin/android/StorageTests.kt
@@ -14,7 +14,6 @@ import com.segment.analytics.kotlin.core.UserInfo
 import com.segment.analytics.kotlin.core.emptyJsonObject
 import kotlinx.coroutines.test.UnconfinedTestDispatcher
 import kotlinx.coroutines.test.runTest
-import kotlinx.serialization.decodeFromString
 import kotlinx.serialization.encodeToString
 import kotlinx.serialization.json.Json
 import kotlinx.serialization.json.JsonObject
@@ -59,7 +58,7 @@ class StorageTests {
                     configuration = Configuration("123"),
                     settings = Settings(),
                     running = false,
-                    initialSettingsDispatched = false,
+                    initializedPlugins = setOf(),
                     enabled = true
                 )
             )
@@ -129,7 +128,7 @@ class StorageTests {
                             edgeFunction = emptyJsonObject
                         ),
                         running = false,
-                        initialSettingsDispatched = false,
+                        initializedPlugins = setOf(),
                         enabled = true
                     )
                 }
@@ -154,7 +153,7 @@ class StorageTests {
         fun `system reset action removes system`() = runTest {
             val action = object : Action<System> {
                 override fun reduce(state: System): System {
-                    return System(state.configuration, null, state.running, state.initialSettingsDispatched, state.enabled)
+                    return System(state.configuration, null, state.running, state.initializedPlugins, state.enabled)
                 }
             }
             store.dispatch(action, System::class)
diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt b/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt
@@ -11,7 +11,6 @@ import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 import kotlinx.serialization.DeserializationStrategy
 import kotlinx.serialization.Serializable
-import kotlinx.serialization.decodeFromString
 import kotlinx.serialization.json.Json
 import kotlinx.serialization.json.JsonObject
 import kotlinx.serialization.serializer
@@ -42,12 +41,22 @@ data class Settings(
     }
 }
 
-internal fun Analytics.update(settings: Settings, type: Plugin.UpdateType) {
+internal suspend fun Analytics.update(settings: Settings) {
+    val systemState = store.currentState(System::class) ?: return
+    val set = mutableSetOf<Int>()
     timeline.applyClosure { plugin ->
         // tell all top level plugins to update.
         // For destination plugins they auto-handle propagation to sub-plugins
+        val type: Plugin.UpdateType =
+            if (systemState.initializedPlugins.contains(plugin.hashCode())) {
+                Plugin.UpdateType.Refresh
+            } else {
+                set.add(plugin.hashCode())
+                Plugin.UpdateType.Initial
+            }
         plugin.update(settings, type)
     }
+    store.dispatch(System.AddInitializedPlugins(set), System::class)
 }
 
 /**
@@ -74,14 +83,7 @@ suspend fun Analytics.checkSettings() {
     val writeKey = configuration.writeKey
     val cdnHost = configuration.cdnHost
 
-    // check current system state to determine whether it's initial or refresh
-    val systemState = store.currentState(System::class) ?: return
-    val updateType = if (systemState.initialSettingsDispatched) {
-        Plugin.UpdateType.Refresh
-    } else {
-        Plugin.UpdateType.Initial
-    }
-
+    store.currentState(System::class) ?: return
     store.dispatch(System.ToggleRunningAction(running = false), System::class)
 
     withContext(networkIODispatcher) {
@@ -92,8 +94,7 @@ suspend fun Analytics.checkSettings() {
             settingsObj?.let {
                 log("Dispatching update settings on ${Thread.currentThread().name}")
                 store.dispatch(System.UpdateSettingsAction(settingsObj), System::class)
-                update(settingsObj, updateType)
-                store.dispatch(System.ToggleSettingsDispatch(dispatched = true), System::class)
+                update(settingsObj)
             }
 
             // we're good to go back to a running state.
diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/State.kt b/core/src/main/java/com/segment/analytics/kotlin/core/State.kt
@@ -1,14 +1,13 @@
 package com.segment.analytics.kotlin.core
 
 import com.segment.analytics.kotlin.core.utilities.putAll
-import kotlinx.serialization.decodeFromString
 import kotlinx.serialization.json.Json
 import kotlinx.serialization.json.JsonObject
 import kotlinx.serialization.json.buildJsonObject
 import kotlinx.serialization.json.put
 import sovran.kotlin.Action
 import sovran.kotlin.State
-import java.util.UUID
+import java.util.*
 
 /**
  * Stores state related to the analytics system
@@ -20,7 +19,7 @@ data class System(
     var configuration: Configuration = Configuration(""),
     var settings: Settings?,
     var running: Boolean,
-    var initialSettingsDispatched: Boolean,
+    var initializedPlugins: Set<Int>,
     var enabled: Boolean
 ) : State {
 
@@ -38,7 +37,7 @@ data class System(
                 configuration = configuration,
                 settings = settings,
                 running = false,
-                initialSettingsDispatched = false,
+                initializedPlugins = setOf(),
                 enabled = true
             )
         }
@@ -50,7 +49,7 @@ data class System(
                 state.configuration,
                 settings,
                 state.running,
-                state.initialSettingsDispatched,
+                state.initializedPlugins,
                 state.enabled
             )
         }
@@ -62,7 +61,7 @@ data class System(
                 state.configuration,
                 state.settings,
                 running,
-                state.initialSettingsDispatched,
+                state.initializedPlugins,
                 state.enabled
             )
         }
@@ -81,21 +80,22 @@ data class System(
                 state.configuration,
                 newSettings,
                 state.running,
-                state.initialSettingsDispatched,
+                state.initializedPlugins,
                 state.enabled
             )
         }
     }
 
-    class ToggleSettingsDispatch(
-        var dispatched: Boolean,
+    class AddInitializedPlugins(
+        var dispatched: Set<Int>,
     ) : Action<System> {
         override fun reduce(state: System): System {
+            val initializedPlugins = state.initializedPlugins + dispatched
             return System(
                 state.configuration,
                 state.settings,
                 state.running,
-                dispatched,
+                initializedPlugins,
                 state.enabled
             )
         }
@@ -107,7 +107,7 @@ data class System(
                 state.configuration,
                 state.settings,
                 state.running,
-                state.initialSettingsDispatched,
+                state.initializedPlugins,
                 enabled
             )
         }
diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/platform/Timeline.kt b/core/src/main/java/com/segment/analytics/kotlin/core/platform/Timeline.kt
@@ -6,7 +6,6 @@ import com.segment.analytics.kotlin.core.System
 import com.segment.analytics.kotlin.core.platform.plugins.logger.segmentLog
 import com.segment.analytics.kotlin.core.reportInternalError
 import kotlinx.coroutines.launch
-import java.util.concurrent.CopyOnWriteArrayList
 import kotlin.reflect.KClass
 
 // Platform abstraction for managing all plugins and their execution
@@ -74,12 +73,12 @@ internal class Timeline {
                 val systemState = store.currentState(System::class)
                 val systemSettings = systemState?.settings
                 systemSettings?.let {
-                    // if we have settings then update plugin with it
-                    plugin.update(it, Plugin.UpdateType.Initial)
-
-                    if (!systemState.initialSettingsDispatched) {
+                    if (systemState.initializedPlugins.isNotEmpty()) {
+                        // if we have settings then update plugin with it
+                        // otherwise it will be updated when settings becomes available
+                        plugin.update(it, Plugin.UpdateType.Initial)
                         store.dispatch(
-                            System.ToggleSettingsDispatch(dispatched = true),
+                            System.AddInitializedPlugins(setOf(plugin.hashCode())),
                             System::class
                         )
                     }
diff --git a/core/src/test/kotlin/com/segment/analytics/kotlin/core/AnalyticsTests.kt b/core/src/test/kotlin/com/segment/analytics/kotlin/core/AnalyticsTests.kt
@@ -809,7 +809,7 @@ class AnalyticsTests {
             segmentDestination.execute(any())
         }
         verify { segmentDestination.onEnableToggled(capture(state)) }
-        assertEquals(false, state[1].enabled)
+        assertEquals(false, state.last().enabled)
 
         analytics.enabled = true
         analytics.track("test")
@@ -818,7 +818,7 @@ class AnalyticsTests {
             segmentDestination.execute(any())
         }
         verify { segmentDestination.onEnableToggled(capture(state)) }
-        assertEquals(true, state[2].enabled)
+        assertEquals(true, state.last().enabled)
     }
 
     @Test
diff --git a/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt b/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt
@@ -85,6 +85,69 @@ class SettingsTests {
         }
     }
 
+    @Test
+    fun `plugin added before settings is available updates plugin correctly`() = runTest {
+        // forces settings to fail
+        mockHTTPClient("")
+
+        analytics = testAnalytics(Configuration(
+            writeKey = "123",
+            application = "Test",
+            autoAddSegmentDestination = false
+        ), testScope, testDispatcher)
+        val mockPlugin = spyk<StubPlugin>()
+
+        // no settings available, should not be called
+        analytics.add(mockPlugin)
+        verify (exactly = 0){
+            mockPlugin.update(any(), any())
+        }
+
+        // load settings
+        mockHTTPClient()
+        analytics.checkSettings()
+        verify (exactly =  1) {
+            mockPlugin.update(any(), Plugin.UpdateType.Initial)
+        }
+        val system = analytics.store.currentState(System::class)
+        assertTrue(system!!.initializedPlugins.contains(mockPlugin.hashCode()))
+
+        // load settings again
+        mockHTTPClient()
+        analytics.checkSettings()
+        verify (exactly =  1) {
+            mockPlugin.update(any(), Plugin.UpdateType.Refresh)
+        }
+    }
+
+    @Test
+    fun `plugin added after settings is available updates plugin correctly`() = runTest {
+        // load settings
+        mockHTTPClient()
+        analytics = testAnalytics(Configuration(
+            writeKey = "123",
+            application = "Test",
+            autoAddSegmentDestination = false
+        ), testScope, testDispatcher)
+        val mockPlugin = spyk<StubPlugin>()
+
+        analytics.add(mockPlugin)
+
+        // settings is already available, update with Initial
+        verify (exactly =  1) {
+            mockPlugin.update(any(), Plugin.UpdateType.Initial)
+        }
+        val system = analytics.store.currentState(System::class)
+        assertTrue(system!!.initializedPlugins.contains(mockPlugin.hashCode()))
+
+        // load settings again
+        mockHTTPClient()
+        analytics.checkSettings()
+        verify (exactly =  1) {
+            mockPlugin.update(any(), Plugin.UpdateType.Refresh)
+        }
+    }
+
     @Test
     fun `isDestinationEnabled returns true when present`() {
         val settings = Settings(
@@ -146,7 +209,7 @@ class SettingsTests {
 
     @Disabled
     @Test
-    fun `can manually enable destinations`() {
+    fun `can manually enable destinations`() = runTest {
         val settings = Settings(
             integrations = buildJsonObject {
                 put("Foo", buildJsonObject {
@@ -168,7 +231,7 @@ class SettingsTests {
         }
 
         analytics.add(barDestination)
-        analytics.update(settings, Plugin.UpdateType.Initial)
+        analytics.update(settings)
 
         analytics.track("track", buildJsonObject { put("direct", true) })
         assertEquals(0, eventCounter.get())
diff --git a/core/src/test/kotlin/com/segment/analytics/kotlin/core/compat/JavaAnalyticsTest.kt b/core/src/test/kotlin/com/segment/analytics/kotlin/core/compat/JavaAnalyticsTest.kt
@@ -608,7 +608,7 @@ internal class JavaAnalyticsTest {
             segmentDestination.execute(any())
         }
         verify { segmentDestination.onEnableToggled(capture(state)) }
-        assertEquals(false, state[1].enabled)
+        assertEquals(false, state.last().enabled)
 
         analytics.enabled = true
         analytics.track("test")
@@ -617,7 +617,7 @@ internal class JavaAnalyticsTest {
             segmentDestination.execute(any())
         }
         verify { segmentDestination.onEnableToggled(capture(state)) }
-        assertEquals(true, state[2].enabled)
+        assertEquals(true, state.last().enabled)
     }
 
     private fun BaseEvent.populate() = apply {
diff --git a/core/src/test/kotlin/com/segment/analytics/kotlin/core/utilities/StorageImplTest.kt b/core/src/test/kotlin/com/segment/analytics/kotlin/core/utilities/StorageImplTest.kt
@@ -12,7 +12,6 @@ import com.segment.analytics.kotlin.core.utils.spyStore
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.UnconfinedTestDispatcher
 import kotlinx.coroutines.test.runTest
-import kotlinx.serialization.decodeFromString
 import kotlinx.serialization.encodeToString
 import kotlinx.serialization.json.Json
 import kotlinx.serialization.json.JsonObject
@@ -56,7 +55,7 @@ internal class StorageImplTest {
                 configuration = Configuration("123"),
                 settings = Settings(),
                 running = false,
-                initialSettingsDispatched = false,
+                initializedPlugins = setOf(),
                 enabled = true
             )
         )
@@ -126,7 +125,7 @@ internal class StorageImplTest {
                         middlewareSettings = emptyJsonObject
                     ),
                     running = false,
-                    initialSettingsDispatched = false,
+                    initializedPlugins = setOf(),
                     enabled = true
                 )
             }
@@ -152,7 +151,7 @@ internal class StorageImplTest {
     fun `system reset action removes system`() = runTest {
         val action = object : Action<System> {
             override fun reduce(state: System): System {
-                return System(state.configuration, null, state.running, state.initialSettingsDispatched, state.enabled)
+                return System(state.configuration, null, state.running, state.initializedPlugins, state.enabled)
             }
         }
         store.dispatch(action, System::class)

Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,6 @@ import com.segment.analytics.kotlin.core.UserInfo`
`14`	`14`	`import com.segment.analytics.kotlin.core.emptyJsonObject`
`15`	`15`	`import kotlinx.coroutines.test.UnconfinedTestDispatcher`
`16`	`16`	`import kotlinx.coroutines.test.runTest`
`17`		`-import kotlinx.serialization.decodeFromString`
`18`	`17`	`import kotlinx.serialization.encodeToString`
`19`	`18`	`import kotlinx.serialization.json.Json`
`20`	`19`	`import kotlinx.serialization.json.JsonObject`
`@@ -59,7 +58,7 @@ class StorageTests {`
`59`	`58`	`configuration = Configuration("123"),`
`60`	`59`	`settings = Settings(),`
`61`	`60`	`running = false,`
`62`		`- initialSettingsDispatched = false,`
	`61`	`+ initializedPlugins = setOf(),`
`63`	`62`	`enabled = true`
`64`	`63`	`)`
`65`	`64`	`)`
`@@ -129,7 +128,7 @@ class StorageTests {`
`129`	`128`	`edgeFunction = emptyJsonObject`
`130`	`129`	`),`
`131`	`130`	`running = false,`
`132`		`- initialSettingsDispatched = false,`
	`131`	`+ initializedPlugins = setOf(),`
`133`	`132`	`enabled = true`
`134`	`133`	`)`
`135`	`134`	`}`
`@@ -154,7 +153,7 @@ class StorageTests {`
`154`	`153`	fun `system reset action removes system`() = runTest {
`155`	`154`	`val action = object : Action<System> {`
`156`	`155`	`override fun reduce(state: System): System {`
`157`		`- return System(state.configuration, null, state.running, state.initialSettingsDispatched, state.enabled)`
	`156`	`+ return System(state.configuration, null, state.running, state.initializedPlugins, state.enabled)`
`158`	`157`	`}`
`159`	`158`	`}`
`160`	`159`	`store.dispatch(action, System::class)`
Original file line number	Diff line number	Diff line change
`@@ -809,7 +809,7 @@ class AnalyticsTests {`
`809`	`809`	`segmentDestination.execute(any())`
`810`	`810`	`}`
`811`	`811`	`verify { segmentDestination.onEnableToggled(capture(state)) }`
`812`		`- assertEquals(false, state[1].enabled)`
	`812`	`+ assertEquals(false, state.last().enabled)`
`813`	`813`
`814`	`814`	`analytics.enabled = true`
`815`	`815`	`analytics.track("test")`
`@@ -818,7 +818,7 @@ class AnalyticsTests {`
`818`	`818`	`segmentDestination.execute(any())`
`819`	`819`	`}`
`820`	`820`	`verify { segmentDestination.onEnableToggled(capture(state)) }`
`821`		`- assertEquals(true, state[2].enabled)`
	`821`	`+ assertEquals(true, state.last().enabled)`
`822`	`822`	`}`
`823`	`823`
`824`	`824`	`@Test`
Original file line number	Diff line number	Diff line change
`@@ -608,7 +608,7 @@ internal class JavaAnalyticsTest {`
`608`	`608`	`segmentDestination.execute(any())`
`609`	`609`	`}`
`610`	`610`	`verify { segmentDestination.onEnableToggled(capture(state)) }`
`611`		`- assertEquals(false, state[1].enabled)`
	`611`	`+ assertEquals(false, state.last().enabled)`
`612`	`612`
`613`	`613`	`analytics.enabled = true`
`614`	`614`	`analytics.track("test")`
`@@ -617,7 +617,7 @@ internal class JavaAnalyticsTest {`
`617`	`617`	`segmentDestination.execute(any())`
`618`	`618`	`}`
`619`	`619`	`verify { segmentDestination.onEnableToggled(capture(state)) }`
`620`		`- assertEquals(true, state[2].enabled)`
	`620`	`+ assertEquals(true, state.last().enabled)`
`621`	`621`	`}`
`622`	`622`
`623`	`623`	`private fun BaseEvent.populate() = apply {`
Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,6 @@ import com.segment.analytics.kotlin.core.utils.spyStore`
`12`	`12`	`import kotlinx.coroutines.test.TestScope`
`13`	`13`	`import kotlinx.coroutines.test.UnconfinedTestDispatcher`
`14`	`14`	`import kotlinx.coroutines.test.runTest`
`15`		`-import kotlinx.serialization.decodeFromString`
`16`	`15`	`import kotlinx.serialization.encodeToString`
`17`	`16`	`import kotlinx.serialization.json.Json`
`18`	`17`	`import kotlinx.serialization.json.JsonObject`
`@@ -56,7 +55,7 @@ internal class StorageImplTest {`
`56`	`55`	`configuration = Configuration("123"),`
`57`	`56`	`settings = Settings(),`
`58`	`57`	`running = false,`
`59`		`- initialSettingsDispatched = false,`
	`58`	`+ initializedPlugins = setOf(),`
`60`	`59`	`enabled = true`
`61`	`60`	`)`
`62`	`61`	`)`
`@@ -126,7 +125,7 @@ internal class StorageImplTest {`
`126`	`125`	`middlewareSettings = emptyJsonObject`
`127`	`126`	`),`
`128`	`127`	`running = false,`
`129`		`- initialSettingsDispatched = false,`
	`128`	`+ initializedPlugins = setOf(),`
`130`	`129`	`enabled = true`
`131`	`130`	`)`
`132`	`131`	`}`
`@@ -152,7 +151,7 @@ internal class StorageImplTest {`
`152`	`151`	fun `system reset action removes system`() = runTest {
`153`	`152`	`val action = object : Action<System> {`
`154`	`153`	`override fun reduce(state: System): System {`
`155`		`- return System(state.configuration, null, state.running, state.initialSettingsDispatched, state.enabled)`
	`154`	`+ return System(state.configuration, null, state.running, state.initializedPlugins, state.enabled)`
`156`	`155`	`}`
`157`	`156`	`}`
`158`	`157`	`store.dispatch(action, System::class)`