Skip to content

Commit

Permalink
fix: Amplitude.build() is not main thread safe on Android (#55)
Browse files Browse the repository at this point in the history
* put disk reads in coroutines

* move storage to build() to avoid disk read on main thread when init

* add isBuilt to core and check before set userid using idContainer
  • Loading branch information
bohan-amplitude authored Aug 6, 2022
1 parent 20bf7ef commit 42ad931
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 36 deletions.
44 changes: 21 additions & 23 deletions android/src/main/java/com/amplitude/android/Amplitude.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import com.amplitude.id.FileIdentityStorageProvider
import com.amplitude.id.IdentityConfiguration
import com.amplitude.id.IdentityContainer
import com.amplitude.id.IdentityUpdateType
import kotlinx.coroutines.async
import kotlinx.coroutines.launch

open class Amplitude(
Expand All @@ -27,36 +28,33 @@ open class Amplitude(
private lateinit var androidContextPlugin: AndroidContextPlugin

override fun build() {
val storageDirectory = (configuration as Configuration).context.getDir("${FileStorage.STORAGE_PREFIX}-${configuration.instanceName}", Context.MODE_PRIVATE)
idContainer = IdentityContainer.getInstance(
IdentityConfiguration(
instanceName = configuration.instanceName,
apiKey = configuration.apiKey,
identityStorageProvider = FileIdentityStorageProvider(),
storageDirectory = storageDirectory
val client = this
isBuilt = amplitudeScope.async(amplitudeDispatcher) {
storage = configuration.storageProvider.getStorage(client)
val storageDirectory = (configuration as Configuration).context.getDir("${FileStorage.STORAGE_PREFIX}-${configuration.instanceName}", Context.MODE_PRIVATE)
idContainer = IdentityContainer.getInstance(
IdentityConfiguration(
instanceName = configuration.instanceName,
apiKey = configuration.apiKey,
identityStorageProvider = FileIdentityStorageProvider(),
storageDirectory = storageDirectory
)
)
)
val listener = AnalyticsIdentityListener(store)
idContainer.identityManager.addIdentityListener(listener)
if (idContainer.identityManager.isInitialized()) {
listener.onIdentityChanged(idContainer.identityManager.getIdentity(), IdentityUpdateType.Initialized)
}
amplitudeScope.launch(amplitudeDispatcher) {
previousSessionId = storage.read(Storage.Constants.PREVIOUS_SESSION_ID) ?.let {
it.toLong()
} ?: -1
val listener = AnalyticsIdentityListener(store)
idContainer.identityManager.addIdentityListener(listener)
if (idContainer.identityManager.isInitialized()) {
listener.onIdentityChanged(idContainer.identityManager.getIdentity(), IdentityUpdateType.Initialized)
}
previousSessionId = storage.read(Storage.Constants.PREVIOUS_SESSION_ID)?.toLong() ?: -1
if (previousSessionId >= 0) {
sessionId = previousSessionId
}
lastEventId = storage.read(Storage.Constants.LAST_EVENT_ID) ?. let {
it.toLong()
} ?: 0
lastEventTime = storage.read(Storage.Constants.LAST_EVENT_TIME) ?. let {
it.toLong()
} ?: -1
lastEventId = storage.read(Storage.Constants.LAST_EVENT_ID)?.toLong() ?: 0
lastEventTime = storage.read(Storage.Constants.LAST_EVENT_TIME)?.toLong() ?: -1
androidContextPlugin = AndroidContextPlugin()
add(androidContextPlugin)
add(AndroidLifecyclePlugin())
true
}
add(AmplitudeDestination())
}
Expand Down
20 changes: 11 additions & 9 deletions android/src/test/java/com/amplitude/android/AmplitudeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,17 @@ class AmplitudeTest {
amplitudeDispatcherField.isAccessible = true
amplitudeDispatcherField.set(amplitude, dispatcher)

amplitude?.setUserId("test user")
amplitude?.setDeviceId("test device")
advanceUntilIdle()
Assertions.assertEquals("test user", amplitude?.store?.userId)
Assertions.assertEquals("test device", amplitude?.store?.deviceId)
if (amplitude?.isBuilt!!.await()) {
amplitude?.setUserId("test user")
amplitude?.setDeviceId("test device")
advanceUntilIdle()
Assertions.assertEquals("test user", amplitude?.store?.userId)
Assertions.assertEquals("test device", amplitude?.store?.deviceId)

amplitude?.reset()
advanceUntilIdle()
Assertions.assertNull(amplitude?.store?.userId)
Assertions.assertNotEquals("test device", amplitude?.store?.deviceId)
amplitude?.reset()
advanceUntilIdle()
Assertions.assertNull(amplitude?.store?.userId)
Assertions.assertNotEquals("test device", amplitude?.store?.deviceId)
}
}
}
14 changes: 10 additions & 4 deletions core/src/main/java/com/amplitude/core/Amplitude.kt
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import com.amplitude.id.IdentityContainer
import com.amplitude.id.IdentityUpdateType
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import java.util.UUID
import java.util.concurrent.Executors
Expand All @@ -45,14 +47,14 @@ open class Amplitude internal constructor(
val retryDispatcher: CoroutineDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher()
) {
internal val timeline: Timeline
val storage: Storage
lateinit var storage: Storage
val logger: Logger
protected lateinit var idContainer: IdentityContainer
lateinit var isBuilt: Deferred<Boolean>

init {
require(configuration.isValid()) { "invalid configuration" }
timeline = Timeline().also { it.amplitude = this }
storage = configuration.storageProvider.getStorage(this)
logger = configuration.loggerProvider.getLogger(this)
build()
}
Expand All @@ -63,6 +65,7 @@ open class Amplitude internal constructor(
constructor(configuration: Configuration) : this(configuration, State())

open fun build() {
storage = configuration.storageProvider.getStorage(this)
idContainer = IdentityContainer.getInstance(IdentityConfiguration(instanceName = configuration.instanceName, apiKey = configuration.apiKey, identityStorageProvider = IMIdentityStorageProvider()))
val listener = AnalyticsIdentityListener(store)
idContainer.identityManager.addIdentityListener(listener)
Expand All @@ -73,7 +76,8 @@ open class Amplitude internal constructor(
add(ContextPlugin())
add(AmplitudeDestination())

amplitudeScope.launch(amplitudeDispatcher) {
isBuilt = amplitudeScope.async(amplitudeDispatcher) {
true
}
}

Expand Down Expand Up @@ -166,7 +170,9 @@ open class Amplitude internal constructor(
*/
fun setUserId(userId: String?): Amplitude {
amplitudeScope.launch(amplitudeDispatcher) {
idContainer.identityManager.editIdentity().setUserId(userId).commit()
if (isBuilt.await()) {
idContainer.identityManager.editIdentity().setUserId(userId).commit()
}
}
return this
}
Expand Down

0 comments on commit 42ad931

Please sign in to comment.