Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Notifications] Fix for ConcurrentModificationException while handling notification #12646

Merged
merged 6 commits into from
Sep 20, 2024
Merged

[Notifications] Fix for ConcurrentModificationException while handling notification #12646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6771316
Write a failing test to ensure we have a bug - ConcurrentModification…
AnirudhBhat Sep 20, 2024
3634356
Add Synchronized annotation for removeNotificationByNotificationIdFro…
AnirudhBhat Sep 20, 2024
b0c1f65
Add Synchronized annotation for few other methods.
AnirudhBhat Sep 20, 2024
c63e0b0
Uncomment code changes
AnirudhBhat Sep 20, 2024
c0c13f6
Test name change
AnirudhBhat Sep 20, 2024
adb2e56
Add release notes
AnirudhBhat Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----
Expand Down
5 changes: 5 additions & 0 deletions .../src/main/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -221,6 +223,7 @@ class NotificationMessageHandler @Inject constructor(
notificationBuilder.cancelAllNotifications()
}

@Synchronized
fun removeNotificationByRemoteIdFromSystemsBar(remoteNoteId: Long) {
val keptNotifs = HashMap<Int, Notification>()
ACTIVE_NOTIFICATIONS_MAP.asSequence()
Expand All @@ -237,6 +240,7 @@ class NotificationMessageHandler @Inject constructor(
updateNotificationsState()
}

@Synchronized
fun removeNotificationByNotificationIdFromSystemsBar(localPushId: Int) {
Copy link
Contributor Author

@AnirudhBhat AnirudhBhat Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use ConcurrentHashMap instead of MutableMap and remove the @Synchronized methods, and that would work too. However, given that concurrent access to notifications is infrequent and only happens during specific notification-related events, I opted to use the @Synchronized annotation for simplicity and minimal overhead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra info! I agree, additional argument for using synchronized is that we used to use it before this PR which likely introduced this crash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnirudhBhat @malinajirka 👋

I would suggest to go with the thread-safe map:

  • That's probably faster as we don't lock the whole object, but this is not really important
  • More important, I think it is harder to break, because those @synchronized here are not really clear why we have them here, and I think that's why they were removed. Alos, removing just 1 synchronized from any public method will return the crash, or the worst, adding a new public method that touches the map will also cause the same issue again

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnirudhBhat also kudos for the unit test!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kidinov Thanks for the input and you raise a valid point here. My train of though was that

  1. Adding @synchronized annotation would make it explicit that we are dealing with concurrency here and need to be careful of the modifications.
  2. While ConcurrentHashMap is thread safe in general. Some operation do require careful consideration before usage - especially operations that checks existing key and then update its value as these are not atomic and need to be refactored to use other methods like putIfAbsent.

Overall. I agree with you that with this approach, it's easy to make a mistake.I don't have a strong preference, and I can replace this implementation with ConcurrentHashMap in a separate PR.

Copy link
Contributor

@kidinov kidinov Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked up the code again, and actually, we already have public methods that don't have synchronised on them and that modify the map

    fun removeAllNotificationsFromSystemsBar() {
        clearNotifications()
        notificationBuilder.cancelAllNotifications()
    }

    fun markNotificationsOfTypeTapped(type: NotificationChannelType) {
        ACTIVE_NOTIFICATIONS_MAP.asSequence()
            .filter { it.value.channelType == type }
            .forEach { row ->
                analyticsTracker.trackNotificationAnalytics(PUSH_NOTIFICATION_TAPPED, row.value)
                analyticsTracker.flush()
            }
    }

So maybe the crash still can happen even now

Also, interestingly, even during writing the original code, it was considered that concurrent modification is possible, but no generic solution was applied

        // Using a copy of the map to avoid concurrency problems
        ACTIVE_NOTIFICATIONS_MAP.toMap().asSequence().forEach { row ->

ah, and ACTIVE_NOTIFICATIONS_MAP is not a constant, so it doesn't follow the naming convention

So overall yes, I'd suggest:

  • Use concurrent map
  • Rename it to follow the convention
  • Remove unnecessary copies of the map
  • Maybe extend the test to call all the public methods to validate the solution

val keptNotifs = HashMap<Int, Notification>()
ACTIVE_NOTIFICATIONS_MAP.asSequence()
Expand All @@ -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
Expand Down
31 changes: 31 additions & 0 deletions .../test/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandlerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down
Loading