Skip to content

Commit

Permalink
fix race condition that causes plugin update with incorrect type (#196)
Browse files Browse the repository at this point in the history 
* add initializedPlugins to system state

* fix broken tests

* add unit tests
  • Loading branch information
@wenxi-zeng
wenxi-zeng authored Nov 3, 2023
1 parent 895c3ab commit 8f720da
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 43 deletions.
7 changes: 3 additions & 4 deletions android/src/test/java/com/segment/analytics/kotlin/android/StorageTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -59,7 +58,7 @@ class StorageTests {
configuration = Configuration("123"),
settings = Settings(),
running = false,
initialSettingsDispatched = false,
initializedPlugins = setOf(),
enabled = true
)
)
Expand Down Expand Up @@ -129,7 +128,7 @@ class StorageTests {
edgeFunction = emptyJsonObject
),
running = false,
initialSettingsDispatched = false,
initializedPlugins = setOf(),
enabled = true
)
}
Expand All @@ -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)
Expand Down
25 changes: 13 additions & 12 deletions core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

/**
Expand All @@ -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) {
Expand All @@ -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.
Expand Down
22 changes: 11 additions & 11 deletions core/src/main/java/com/segment/analytics/kotlin/core/State.kt
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 {

Expand All @@ -38,7 +37,7 @@ data class System(
configuration = configuration,
settings = settings,
running = false,
initialSettingsDispatched = false,
initializedPlugins = setOf(),
enabled = true
)
}
Expand All @@ -50,7 +49,7 @@ data class System(
state.configuration,
settings,
state.running,
state.initialSettingsDispatched,
state.initializedPlugins,
state.enabled
)
}
Expand All @@ -62,7 +61,7 @@ data class System(
state.configuration,
state.settings,
running,
state.initialSettingsDispatched,
state.initializedPlugins,
state.enabled
)
}
Expand All @@ -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
)
}
Expand All @@ -107,7 +107,7 @@ data class System(
state.configuration,
state.settings,
state.running,
state.initialSettingsDispatched,
state.initializedPlugins,
enabled
)
}
Expand Down
11 changes: 5 additions & 6 deletions core/src/main/java/com/segment/analytics/kotlin/core/platform/Timeline.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
)
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/kotlin/com/segment/analytics/kotlin/core/AnalyticsTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down
67 changes: 65 additions & 2 deletions core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand All @@ -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())
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/kotlin/com/segment/analytics/kotlin/core/compat/JavaAnalyticsTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions core/src/test/kotlin/com/segment/analytics/kotlin/core/utilities/StorageImplTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -56,7 +55,7 @@ internal class StorageImplTest {
configuration = Configuration("123"),
settings = Settings(),
running = false,
initialSettingsDispatched = false,
initializedPlugins = setOf(),
enabled = true
)
)
Expand Down Expand Up @@ -126,7 +125,7 @@ internal class StorageImplTest {
middlewareSettings = emptyJsonObject
),
running = false,
initialSettingsDispatched = false,
initializedPlugins = setOf(),
enabled = true
)
}
Expand All @@ -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)
Expand Down

0 comments on commit 8f720da

Please sign in to comment.