[Notifications] Fix for ConcurrentModificationException while handling notification

[AnirudhBhat]

Closes: #12636

Description

This PR addresses a crash peaMlT-TT-p2 related to notifications. While I couldn't reproduce the crash directly, I was able to analyze the stacktrace, identify the root cause, and reproduce the issue through a unit test.

The crash occurred because ACTIVE_NOTIFICATIONS_MAP was being modified concurrently by multiple threads, leading to a ConcurrentModificationException.

For example - In ReviewDetailViewModel, while loading product reviews, the map is modified on a background thread when markReviewAsSeen(remoteReviewId, it) is called. Simultaneously, if a new notification arrives or the user dismisses an existing notification, we attempt to modify the same ACTIVE_NOTIFICATIONS_MAP, which could lead to a ConcurrentModificationException.

The issue is resolved by adding the @Synchronized annotation, ensuring that only one thread can modify ACTIVE_NOTIFICATIONS_MAP at a time. After applying this fix, the unit test passes successfully.

Steps to reproduce

1. Remove the @Synchronized annotation from removeNotificationByNotificationIdFromSystemsBar method
2. Run remove notifications concurrently without throwing ConcurrentModificationException unit test
3. Ensure the test fails with ConcurrentModificationException

Testing information

I couldn't test this with real notifications, but the fact that the unit test passes after the fix provides enough confidence to assert that the ConcurrentModificationException crash has been resolved for this particular scenario.

• I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

• The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
• Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
• Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

[dangermattic]

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	adb2e56
	Direct Download	woocommerce-wear-prototype-build-pr12646-adb2e56.apk

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	adb2e56
	Direct Download	woocommerce-prototype-build-pr12646-adb2e56.apk

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.64%. Comparing base (4cbfa75) to head (adb2e56).
Report is 7 commits behind head on trunk.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #12646      +/-   ##
============================================
+ Coverage     40.63%   40.64%   +0.01%     
- Complexity     5670     5673       +3     
============================================
  Files          1230     1230              
  Lines         69165    69165              
  Branches       9579     9579              
============================================
+ Hits          28104    28114      +10     
+ Misses        38479    38466      -13     
- Partials       2582     2585       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AnirudhBhat]

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.

[malinajirka]

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.

[kidinov]

@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?

[kidinov]

@AnirudhBhat also kudos for the unit test!

[AnirudhBhat]

@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.

[kidinov]

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

[malinajirka]

Thanks @AnirudhBhat for working on this and providing detailed description! Great job reproducing it in a unit test, that gives me enough confidence these changes actually fix the issue.

I've smoke tested push notifications and they work as expected.