amplitude / Amplitude-Kotlin

Amplitude Kotlin SDK
MIT License
27 stars 10 forks source link

ConcurrentModificationExceptions #115

Closed jp2014 closed 1 year ago

jp2014 commented 1 year ago

Expected Behavior

SDK should not cause ConcurrentModificationExceptions

Current Behavior

SDK Version: 1.7.1

We are seeing a pretty heavy volume of ConcurrentModificationExceptions caused by the Amplitude-Kotlin SDK. We've seen about 5k crashes over the last month related to this.

Fatal Exception: java.util.ConcurrentModificationException:
       at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:760)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:793)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:791)
       at java.util.HashMap.putMapEntries(HashMap.java:511)
       at java.util.HashMap.putAll(HashMap.java:784)
       at com.amplitude.analytics.connector.IdentityStoreImpl$editIdentity$1.updateUserProperties(IdentityStore.kt:75)
       at com.amplitude.android.plugins.AnalyticsConnectorPlugin.execute(AnalyticsConnectorPlugin.kt:45)
       at com.amplitude.core.platform.Mediator.execute(Mediator.java:48)
       at com.amplitude.core.platform.Timeline.applyPlugins(Timeline.kt:37)
       at com.amplitude.core.platform.Timeline.applyPlugins(Timeline.kt:30)
       at com.amplitude.core.platform.Timeline.process(Timeline.kt:16)
       at com.amplitude.android.Timeline.processEventMessage(Timeline.kt:107)
       at com.amplitude.android.Timeline.access$processEventMessage(Timeline.kt:10)
       at com.amplitude.android.Timeline$start$1.invokeSuspend(Timeline.kt:31)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at java.lang.Thread.run(Thread.java:920)
Fatal Exception: java.util.ConcurrentModificationException:
       at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:760)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:792)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:790)
       at java.util.HashMap.putMapEntries(HashMap.java:511)
       at java.util.HashMap.putAll(HashMap.java:784)
       at com.amplitude.core.platform.intercept.IdentifyInterceptFileStorageHandler.fetchAndMergeToIdentifyEvent(IdentifyInterceptFileStorageHandler.kt:75)
       at com.amplitude.core.platform.intercept.IdentifyInterceptor.fetchAndMergeToIdentifyEvent(IdentifyInterceptor.kt:85)
       at com.amplitude.core.platform.intercept.IdentifyInterceptor.intercept(IdentifyInterceptor.kt:65)
       at com.amplitude.core.platform.plugins.AmplitudeDestination$enqueue$1$1.invokeSuspend(AmplitudeDestination.kt:51)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
       at java.lang.Thread.run(Thread.java:1012)
Fatal Exception: java.util.ConcurrentModificationException:
       at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:760)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:792)
       at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:790)
       at com.amplitude.android.plugins.AnalyticsConnectorPlugin.execute(AnalyticsConnectorPlugin.kt:35)
       at com.amplitude.core.platform.Mediator.execute(Mediator.java:48)
       at com.amplitude.core.platform.Timeline.applyPlugins(Timeline.kt:37)
       at com.amplitude.core.platform.Timeline.applyPlugins(Timeline.kt:30)
       at com.amplitude.core.platform.Timeline.process(Timeline.kt:16)
       at com.amplitude.android.Timeline.processEventMessage(Timeline.kt:107)
       at com.amplitude.android.Timeline.access$processEventMessage(Timeline.kt:10)
       at com.amplitude.android.Timeline$start$1.invokeSuspend(Timeline.kt:31)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
       at java.lang.Thread.run(Thread.java:1012)

Possible Solution

My best guess is that com.amplitude.core.platform.Timeline.plugins is not thread safe. Plugins could be added/removed/modified from different threads which would cause this exception.

Steps to Reproduce

I'm unable to reproduce, but we see quite a few reports of this in our error logging.

Environment

image
jp2014 commented 1 year ago

Hello! Any updates here? We are still seeing a lot of crashes in our production app. Thanks!

jp2014 commented 1 year ago

We've updated to the latest 1.8.2 version and continue to see a large amount of crashes. Any updates are appreciated!

justin-fiedler commented 1 year ago

Hi @jp2014 thanks for bringing this to our attention. Unfortunately we have not been able to reproduce this on our end.

The error logs are really helpful. Starting with 1.8.2 you should no longer see second error at com.amplitude.core.platform.intercept.IdentifyInterceptFileStorageHandler.fetchAndMergeToIdentifyEvent.

We will investigate further and try to locate the root cause.

To help us debug can you please provide some more details.

  1. Code snippet of creating your Amplitude instance and the provided Configuration (with PII removed)
  2. What Amplitude SDK methods do you call in your app? track & identify? Code snippets of usage would be useful.
  3. Did you add any custom Plugins?
  4. Are you also using Amplitude Experiment?

Thanks!

jp2014 commented 1 year ago

Here's our wrapper class. It's created as a singleton early in app lifecycle. We use all of the wrapped function in this class. We don't add any plugins to amplitude or wrap it with any plugins. We do use Amplitude Experiment as well and do use the initializeWithAmplitudeAnalytics and automaticFetchOnAmplitudeIdentityChange features. I'm not able to reliably reproduce this on my end either, but I have seen it.

I think it should be reproducible with some test code that concurrently calls Timeline.applyPlugins, Timeline.remove, etc a bunch of times.

Thank you!

class AmplitudeAnalyticsWrapperImpl(token: String, context: Context) : AmplitudeAnalyticsWrapper {
    private val amplitudeClient: Amplitude

    init {
        val config = Configuration(
            apiKey = token,
            context = context,
            trackingSessionEvents = true
        )
        amplitudeClient = Amplitude(config)
    }

    override fun track(key: String, data: Map<String, Any>) {
        safeClientCall {
            amplitudeClient.track(key, data)
        }
    }

    override fun setUserId(id: String) {
        safeClientCall {
            amplitudeClient.setUserId(id)
        }
    }

    override fun identify(identify: Identify) {
        safeClientCall {
            amplitudeClient.identify(identify)
        }
    }

    override fun reset() {
        safeClientCall {
            amplitudeClient.reset()
        }
    }

    override fun revenue(revenue: Revenue) {
        safeClientCall {
            amplitudeClient.revenue(revenue)
        }
    }

    private fun safeClientCall(call: () -> Unit) {
        try {
            call()
        } catch (e: Exception) {
            log(e)
        }
    }
}
justin-fiedler commented 1 year ago

Hi @jp2014 we released v1.9.1 with a potential fix. Can you please update and let us know if that works for you. Thanks!

jp2014 commented 1 year ago

Thanks @justin-fiedler. We are still early in our release, but no crashes so far! Thank you!