Fix ConcurrentModificationException in BitmapPool

jonathanmos · jonathanmos · commit e2a50d3849f4 · 2024-01-02T11:55:37.000+02:00
diff --git a/features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPool.kt b/features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPool.kt
@@ -103,6 +103,21 @@ internal class BitmapPool(
         }?.apply { markBitmapAsUsed(this) }
     }
 
+    override fun onConfigurationChanged(newConfig: Configuration) {}
+
+    @Synchronized
+    override fun onLowMemory() {
+        bitmapPoolHelper.safeCall {
+            @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
+            cache.evictAll()
+        }
+    }
+
+    @Synchronized
+    override fun onTrimMemory(level: Int) {
+        cacheUtils.handleTrimMemory(level, cache)
+    }
+
     internal fun getBitmapByProperties(width: Int, height: Int, config: Config): Bitmap? {
         val key = bitmapPoolHelper.generateKey(width, height, config)
         return get(key)
@@ -146,19 +161,6 @@ internal class BitmapPool(
         }
     }
 
-    override fun onConfigurationChanged(newConfig: Configuration) {}
-
-    override fun onLowMemory() {
-        bitmapPoolHelper.safeCall {
-            @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
-            cache.evictAll()
-        }
-    }
-
-    override fun onTrimMemory(level: Int) {
-        cacheUtils.handleTrimMemory(level, cache)
-    }
-
     internal companion object {
         @VisibleForTesting
         @Suppress("MagicNumber")
diff --git a/features/dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPoolTest.kt b/features/dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPoolTest.kt
@@ -17,7 +17,9 @@ import fr.xgouchet.elmyr.junit5.ForgeConfiguration
 import fr.xgouchet.elmyr.junit5.ForgeExtension
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.RepeatedTest
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
 import org.junit.jupiter.api.extension.ExtendWith
 import org.junit.jupiter.api.extension.Extensions
 import org.mockito.Mock
@@ -32,6 +34,8 @@ import org.mockito.kotlin.verify
 import org.mockito.kotlin.whenever
 import org.mockito.quality.Strictness
 import java.util.concurrent.CountDownLatch
+import java.util.concurrent.Executors
+import java.util.concurrent.Future
 import java.util.concurrent.TimeUnit
 
 @Extensions(
@@ -259,7 +263,7 @@ internal class BitmapPoolTest {
 
         // Then
         countDownLatch.await(5, TimeUnit.SECONDS)
-        assertThat(testedCache.bitmapsBySize.get(key)?.size).isEqualTo(3)
+        assertThat(testedCache.bitmapsBySize[key]?.size).isEqualTo(3)
     }
 
     @Test
@@ -358,6 +362,38 @@ internal class BitmapPoolTest {
         assertThat(captor.firstValue).isEqualTo(0)
     }
 
+    @RepeatedTest(30)
+    fun `M not receive ConcurrentModificationException W get() { and onLowMemory }`(
+        forge: Forge
+    ) {
+        // when this issue occurs, the frequency is roughly once per 3000 runs,
+
+        // Given
+        val numberOfThreads = 3
+        val executor = Executors.newFixedThreadPool(numberOfThreads)
+        val tasks = mutableListOf<Future<*>>()
+
+        val allThreadsStartedLatch = CountDownLatch(numberOfThreads)
+        testedCache.put(mockBitmap)
+        val bitmapKey = spyBitmapPoolHelper.generateKey(mockBitmap)
+
+        // When
+        repeat(numberOfThreads) {
+            tasks += executor.submit {
+                allThreadsStartedLatch.countDown()
+                allThreadsStartedLatch.await()
+                if (forge.aBool()) {
+                    testedCache.get(bitmapKey)
+                } else {
+                    testedCache.onLowMemory()
+                }
+            }
+        }
+
+        // Then
+        tasks.forEach { assertDoesNotThrow { it.get() } }
+    }
+
     // endregion
 
     private fun createMockBitmap(forge: Forge): Bitmap {
