From 8f720da419fdf36af9003e0fa3d0b1d39af6ae0a Mon Sep 17 00:00:00 2001 From: Wenxi Zeng Date: Fri, 3 Nov 2023 13:34:08 -0700 Subject: [PATCH] fix race condition that causes plugin update with incorrect type (#196) * add initializedPlugins to system state * fix broken tests * add unit tests --- .../analytics/kotlin/android/StorageTests.kt | 7 +- .../segment/analytics/kotlin/core/Settings.kt | 25 +++---- .../segment/analytics/kotlin/core/State.kt | 22 +++--- .../kotlin/core/platform/Timeline.kt | 11 ++- .../analytics/kotlin/core/AnalyticsTests.kt | 4 +- .../analytics/kotlin/core/SettingsTests.kt | 67 ++++++++++++++++++- .../kotlin/core/compat/JavaAnalyticsTest.kt | 4 +- .../kotlin/core/utilities/StorageImplTest.kt | 7 +- 8 files changed, 104 insertions(+), 43 deletions(-) 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 index 39657507..d26e6219 100644 --- 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 { 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 index 0a9dcdf3..b07aea1a 100644 --- 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() 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 index 152b8b78..21275a88 100644 --- 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, 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, ) : Action { 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 index adbc1a1e..7113d6f2 100644 --- 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 index b83ff6c8..60ece38f 100644 --- 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 index dd52ac05..6c121ecf 100644 --- 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() + + // 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() + + 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 index b249e046..16b998cc 100644 --- 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 index 70066e71..d3b7c4c6 100644 --- 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 { 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)