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] Replace mutable map with Thread safe map for storing all active notifications #12655

Draft
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Draft

[Notifications] Replace mutable map with Thread safe map for storing all active notifications #12655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2052d1a
Change naming convention for ACTIVE_NOTIFICATIONS_MAP since it is not…
AnirudhBhat Sep 23, 2024
d3919b1
Replace mutableMap with Thread safe map
AnirudhBhat Sep 23, 2024
4ccd47b
Remove @Synchronized annotation
AnirudhBhat Sep 23, 2024
0b4f9be
Remove unnecessary copies of the map
AnirudhBhat Sep 23, 2024
4210103
Update test to handle all notification related public methods to veri…
AnirudhBhat Sep 23, 2024
02a6953
Uncomment code
AnirudhBhat Sep 23, 2024
b30300d
Add release notes
AnirudhBhat Sep 23, 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
2 changes: 1 addition & 1 deletion RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

20.6
-----

- [*][Internal] Replace mutable map with Thread safe map for storing all active notifications [https://github.com/woocommerce/woocommerce-android/pull/12655]

20.5
-----
Expand Down
41 changes: 18 additions & 23 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 @@ -25,6 +25,7 @@ import org.wordpress.android.fluxc.generated.NotificationActionBuilder
import org.wordpress.android.fluxc.model.notification.NotificationModel
import org.wordpress.android.fluxc.store.AccountStore
import org.wordpress.android.fluxc.store.NotificationStore.FetchNotificationPayload
import java.util.concurrent.ConcurrentHashMap
import javax.inject.Inject
import javax.inject.Singleton
import kotlin.random.Random
Expand All @@ -49,15 +50,13 @@ class NotificationMessageHandler @Inject constructor(
@VisibleForTesting
const val MAX_INBOX_ITEMS = 5

private val ACTIVE_NOTIFICATIONS_MAP = mutableMapOf<Int, Notification>()
private val activeNotificationsMap = ConcurrentHashMap<Int, Notification>()
}

@Synchronized
fun onPushNotificationDismissed(notificationId: Int) {
removeNotificationByNotificationIdFromSystemsBar(notificationId)
}

@Synchronized
fun onLocalNotificationDismissed(notificationId: Int, notificationType: String) {
removeNotificationByNotificationIdFromSystemsBar(notificationId)
AnalyticsTracker.track(
Expand Down Expand Up @@ -132,13 +131,13 @@ class NotificationMessageHandler @Inject constructor(
private fun handleWooNotification(notification: Notification) {
val randomNumber = if (notification.noteType == NewOrder) Random.nextInt() else 0
val localPushId = getLocalPushIdForNoteId(notification.remoteNoteId, randomNumber)
ACTIVE_NOTIFICATIONS_MAP[getLocalPushId(localPushId, randomNumber)] = notification
activeNotificationsMap[getLocalPushId(localPushId, randomNumber)] = notification
if (notificationBuilder.isNotificationsEnabled()) {
analyticsTracker.trackNotificationAnalytics(PUSH_NOTIFICATION_RECEIVED, notification)
analyticsTracker.flush()
}

val isGroupNotification = ACTIVE_NOTIFICATIONS_MAP.size > 1
val isGroupNotification = activeNotificationsMap.size > 1
with(notificationBuilder) {
buildAndDisplayWooNotification(
pushId = localPushId,
Expand All @@ -147,7 +146,7 @@ class NotificationMessageHandler @Inject constructor(
)

if (isGroupNotification) {
val notesMap = ACTIVE_NOTIFICATIONS_MAP.toMap()
val notesMap = activeNotificationsMap.toMap()
val message = notesMap.values.take(MAX_INBOX_ITEMS).joinToString("\n") {
it.noteMessage.orEmpty()
}
Expand Down Expand Up @@ -183,23 +182,23 @@ class NotificationMessageHandler @Inject constructor(
// This solution is a temporary HACK to generate a random number for each new order notification
// and use that number to group notifications along with notification_note_id
private fun getLocalPushIdForNoteId(noteId: Long, randomNumber: Int): Int {
for (id in ACTIVE_NOTIFICATIONS_MAP.keys) {
val notification = ACTIVE_NOTIFICATIONS_MAP[getLocalPushId(id, randomNumber)]
for (id in activeNotificationsMap.keys) {
val notification = activeNotificationsMap[getLocalPushId(id, randomNumber)]
if (notification?.remoteNoteId == noteId) {
return id
}
}
return PUSH_NOTIFICATION_ID + ACTIVE_NOTIFICATIONS_MAP.size
return PUSH_NOTIFICATION_ID + activeNotificationsMap.size
}

private fun hasNotifications() = ACTIVE_NOTIFICATIONS_MAP.isNotEmpty()
private fun clearNotifications() = ACTIVE_NOTIFICATIONS_MAP.clear()
private fun hasNotifications() = activeNotificationsMap.isNotEmpty()
private fun clearNotifications() = activeNotificationsMap.clear()

/**
* Find the matching notification and send a track event for [PUSH_NOTIFICATION_TAPPED].
*/
fun markNotificationTapped(remoteNoteId: Long) {
ACTIVE_NOTIFICATIONS_MAP.asSequence()
activeNotificationsMap.asSequence()
.firstOrNull { it.value.remoteNoteId == remoteNoteId }?.let { row ->
analyticsTracker.trackNotificationAnalytics(PUSH_NOTIFICATION_TAPPED, row.value)
analyticsTracker.flush()
Expand All @@ -210,7 +209,7 @@ class NotificationMessageHandler @Inject constructor(
* Loop over all active notifications and send the [PUSH_NOTIFICATION_TAPPED] track event for each one.
*/
fun markNotificationsOfTypeTapped(type: NotificationChannelType) {
ACTIVE_NOTIFICATIONS_MAP.asSequence()
activeNotificationsMap.asSequence()
.filter { it.value.channelType == type }
.forEach { row ->
analyticsTracker.trackNotificationAnalytics(PUSH_NOTIFICATION_TAPPED, row.value)
Expand All @@ -223,10 +222,9 @@ class NotificationMessageHandler @Inject constructor(
notificationBuilder.cancelAllNotifications()
}

@Synchronized
fun removeNotificationByRemoteIdFromSystemsBar(remoteNoteId: Long) {
val keptNotifs = HashMap<Int, Notification>()
ACTIVE_NOTIFICATIONS_MAP.asSequence()
activeNotificationsMap.asSequence()
.forEach { row ->
if (row.value.remoteNoteId == remoteNoteId) {
notificationBuilder.cancelNotification(row.key)
Expand All @@ -236,14 +234,13 @@ class NotificationMessageHandler @Inject constructor(
}

clearNotifications()
ACTIVE_NOTIFICATIONS_MAP.putAll(keptNotifs)
activeNotificationsMap.putAll(keptNotifs)
updateNotificationsState()
}

@Synchronized
fun removeNotificationByNotificationIdFromSystemsBar(localPushId: Int) {
val keptNotifs = HashMap<Int, Notification>()
ACTIVE_NOTIFICATIONS_MAP.asSequence()
activeNotificationsMap.asSequence()
.forEach { row ->
if (row.key == localPushId) {
notificationBuilder.cancelNotification(row.key)
Expand All @@ -253,23 +250,21 @@ class NotificationMessageHandler @Inject constructor(
}

clearNotifications()
ACTIVE_NOTIFICATIONS_MAP.putAll(keptNotifs)
activeNotificationsMap.putAll(keptNotifs)
updateNotificationsState()
}

@Synchronized
fun removeNotificationsOfTypeFromSystemsBar(type: NotificationChannelType, remoteSiteId: Long) {
val keptNotifs = HashMap<Int, Notification>()
// Using a copy of the map to avoid concurrency problems
ACTIVE_NOTIFICATIONS_MAP.toMap().asSequence().forEach { row ->
activeNotificationsMap.asSequence().forEach { row ->
if (row.value.channelType == type && row.value.remoteSiteId == remoteSiteId) {
notificationBuilder.cancelNotification(row.key)
} else {
keptNotifs[row.key] = row.value
}
}
clearNotifications()
ACTIVE_NOTIFICATIONS_MAP.putAll(keptNotifs)
activeNotificationsMap.putAll(keptNotifs)
updateNotificationsState()
}

Expand Down
30 changes: 26 additions & 4 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 @@ -4,6 +4,7 @@ import com.woocommerce.android.R
import com.woocommerce.android.analytics.AnalyticsEvent
import com.woocommerce.android.background.WorkManagerScheduler
import com.woocommerce.android.model.toAppModel
import com.woocommerce.android.notifications.NotificationChannelType
import com.woocommerce.android.notifications.WooNotificationBuilder
import com.woocommerce.android.notifications.push.NotificationTestUtils.TEST_ORDER_NOTE_FULL_DATA_2
import com.woocommerce.android.notifications.push.NotificationTestUtils.TEST_ORDER_NOTE_FULL_DATA_SITE_2
Expand Down Expand Up @@ -464,21 +465,42 @@ class NotificationMessageHandlerTest {
}

@Test
fun `remove notifications concurrently without throwing ConcurrentModificationException`() {
fun `handle notifications concurrently without throwing ConcurrentModificationException`() {
notificationMessageHandler.removeAllNotificationsFromSystemsBar()
val notificationsCount = 100
repeat(notificationsCount) {
notificationMessageHandler.onNewMessageReceived(orderNotificationPayload)
}

runTest {
repeat(50) {
repeat(20) {
launch(Dispatchers.Default) {
notificationMessageHandler.removeNotificationByNotificationIdFromSystemsBar(0)
notificationMessageHandler.markNotificationTapped(0)
}
}

repeat(20) {
launch(Dispatchers.Default) {
notificationMessageHandler.markNotificationsOfTypeTapped(NotificationChannelType.NEW_ORDER)
}
}

repeat(20) {
launch(Dispatchers.Default) {
notificationMessageHandler.removeNotificationByRemoteIdFromSystemsBar(0)
}
}

repeat(20) {
launch(Dispatchers.Default) {
notificationMessageHandler.removeNotificationsOfTypeFromSystemsBar(
NotificationChannelType.NEW_ORDER,
0
)
}
}

repeat(50) {
repeat(20) {
launch(Dispatchers.Default) {
notificationMessageHandler.removeNotificationByNotificationIdFromSystemsBar(0)
}
Expand Down
Loading