Merge pull request #12646 from woocommerce/issue/12636-concurrentmodi…

…ficationexception-notifications [Notifications] Fix for ConcurrentModificationException while handling notification
woocommerce · Sep 20, 2024 · b07aafb · b07aafb
2 parents 4cbfa75 + adb2e56
commit b07aafb
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 0 deletions.
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
@@ -6,6 +6,7 @@
 -----
 - [*] Fixes a bug that prevented users to rename the Product Variation Attributes to because of case insensitive checks [https://github.com/woocommerce/woocommerce-android/pull/12608]
 - [*] Users can directly pick product images when creating Blaze ads [https://github.com/woocommerce/woocommerce-android/pull/12610]
+- [*] Fix for ConcurrentModificationException while removing notification [https://github.com/woocommerce/woocommerce-android/pull/12646]
 
 20.4
 -----

diff --git a/.../src/main/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandler.kt b/.../src/main/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandler.kt
@@ -52,10 +52,12 @@ class NotificationMessageHandler @Inject constructor(
         private val ACTIVE_NOTIFICATIONS_MAP = mutableMapOf<Int, Notification>()
     }
 
+    @Synchronized
     fun onPushNotificationDismissed(notificationId: Int) {
         removeNotificationByNotificationIdFromSystemsBar(notificationId)
     }
 
+    @Synchronized
     fun onLocalNotificationDismissed(notificationId: Int, notificationType: String) {
         removeNotificationByNotificationIdFromSystemsBar(notificationId)
         AnalyticsTracker.track(
@@ -221,6 +223,7 @@ class NotificationMessageHandler @Inject constructor(
         notificationBuilder.cancelAllNotifications()
     }
 
+    @Synchronized
     fun removeNotificationByRemoteIdFromSystemsBar(remoteNoteId: Long) {
         val keptNotifs = HashMap<Int, Notification>()
         ACTIVE_NOTIFICATIONS_MAP.asSequence()
@@ -237,6 +240,7 @@ class NotificationMessageHandler @Inject constructor(
         updateNotificationsState()
     }
 
+    @Synchronized
     fun removeNotificationByNotificationIdFromSystemsBar(localPushId: Int) {
         val keptNotifs = HashMap<Int, Notification>()
         ACTIVE_NOTIFICATIONS_MAP.asSequence()
@@ -253,6 +257,7 @@ class NotificationMessageHandler @Inject constructor(
         updateNotificationsState()
     }
 
+    @Synchronized
     fun removeNotificationsOfTypeFromSystemsBar(type: NotificationChannelType, remoteSiteId: Long) {
         val keptNotifs = HashMap<Int, Notification>()
         // Using a copy of the map to avoid concurrency problems

diff --git a/.../test/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandlerTest.kt b/.../test/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandlerTest.kt
@@ -15,6 +15,11 @@ import com.woocommerce.android.util.NotificationsParser
 import com.woocommerce.android.util.WooLog
 import com.woocommerce.android.util.WooLogWrapper
 import com.woocommerce.android.viewmodel.ResourceProvider
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runTest
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Before
 import org.junit.Test
@@ -38,6 +43,7 @@ import org.wordpress.android.fluxc.model.notification.NotificationModel
 import org.wordpress.android.fluxc.store.AccountStore
 import org.wordpress.android.fluxc.store.NotificationStore.FetchNotificationPayload
 
+@OptIn(ExperimentalCoroutinesApi::class)
 class NotificationMessageHandlerTest {
     private lateinit var notificationMessageHandler: NotificationMessageHandler
 
@@ -457,6 +463,31 @@ class NotificationMessageHandlerTest {
         )
     }
 
+    @Test
+    fun `remove notifications concurrently without throwing ConcurrentModificationException`() {
+        notificationMessageHandler.removeAllNotificationsFromSystemsBar()
+        val notificationsCount = 100
+        repeat(notificationsCount) {
+            notificationMessageHandler.onNewMessageReceived(orderNotificationPayload)
+        }
+
+        runTest {
+            repeat(50) {
+                launch(Dispatchers.Default) {
+                    notificationMessageHandler.removeNotificationByNotificationIdFromSystemsBar(0)
+                }
+            }
+
+            repeat(50) {
+                launch(Dispatchers.Default) {
+                    notificationMessageHandler.removeNotificationByNotificationIdFromSystemsBar(0)
+                }
+            }
+
+            advanceUntilIdle()
+        }
+    }
+
     @Test
     fun `when notification is clicked, then mark new notification as tapped correctly`() {
         notificationMessageHandler.onNewMessageReceived(orderNotificationPayload)