DataDog / dd-sdk-android

Datadog SDK for Android (Compatible with Kotlin and Java)
Apache License 2.0
146 stars 59 forks source link

RUM-4329: Fix `ConcurrentModificationException` during features iteration #2012

Closed 0xnm closed 4 months ago

0xnm commented 4 months ago

What does this PR do?

It is possible to have a crash like that:

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.datadog.android.core.DatadogCore.updateFeatureContext(SourceFile:65)
    at com.datadog.android.rum.internal.domain.scope.RumViewScope.<init>(SourceFile:21)
    at com.datadog.android.rum.internal.domain.scope.RumViewScope.<init>(SourceFile:31)
    at com.datadog.android.rum.internal.domain.scope.RumViewManagerScope.handleEvent(SourceFile:620)
    at com.datadog.android.rum.internal.domain.scope.RumSessionScope.handleEvent(SourceFile:246)
    at com.datadog.android.rum.internal.domain.scope.RumApplicationScope.handleEvent(SourceFile:356)
    at androidx.lifecycle.DispatchQueue$$ExternalSyntheticLambda0.run(SourceFile:278)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:487)
    at java.util.concurrent.FutureTask.run(FutureTask.java:264)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
    at java.lang.Thread.run(Thread.java:1012)

The problem is that DatadogCore.features can be accessed from different threads. This PR makes access for the items of this collection thread-safe.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.69%. Comparing base (0c3e683) to head (29df747).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #2012 +/- ## =========================================== - Coverage 83.76% 83.69% -0.07% =========================================== Files 488 488 Lines 17779 17778 -1 Branches 2667 2667 =========================================== - Hits 14892 14879 -13 - Misses 2174 2178 +4 - Partials 713 721 +8 ``` | [Files](https://app.codecov.io/gh/DataDog/dd-sdk-android/pull/2012?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | Coverage Δ | | |---|---|---| | [...ain/kotlin/com/datadog/android/core/DatadogCore.kt](https://app.codecov.io/gh/DataDog/dd-sdk-android/pull/2012?src=pr&el=tree&filepath=dd-sdk-android-core%2Fsrc%2Fmain%2Fkotlin%2Fcom%2Fdatadog%2Fandroid%2Fcore%2FDatadogCore.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-ZGQtc2RrLWFuZHJvaWQtY29yZS9zcmMvbWFpbi9rb3RsaW4vY29tL2RhdGFkb2cvYW5kcm9pZC9jb3JlL0RhdGFkb2dDb3JlLmt0) | `81.98% <100.00%> (ø)` | | ... and [23 files with indirect coverage changes](https://app.codecov.io/gh/DataDog/dd-sdk-android/pull/2012/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog)
0xnm commented 4 months ago

@xgouchet fair point, I've updated my PR with test.