From 4fdbb1d73851e09c1be88c947ef184d906907a35 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Mon, 29 Apr 2024 17:30:59 -0400 Subject: [PATCH 1/5] move part of operationrepo initialization to a separate thread --- .../internal/operations/impl/OperationRepo.kt | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index ed3158845d..6ba00c3a1e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -69,6 +69,10 @@ internal class OperationRepo( private val executeBucket get() = if (enqueueIntoBucket == 0) 0 else enqueueIntoBucket - 1 + /** + * Perform initialization in background to prevent possible operation point of failures from + * blocking the creation of OperationRepo. + */ init { val executorsMap: MutableMap = mutableMapOf() for (executor in executors) { @@ -78,8 +82,11 @@ internal class OperationRepo( } this.executorsMap = executorsMap - for (operation in _operationModelStore.list()) { - internalEnqueue(OperationQueueItem(operation, bucket = enqueueIntoBucket), flush = false, addToStore = false) + // load operation in a separate thread to avoid halting the main process + coroutineScope.launch { + for (operation in _operationModelStore.list().withIndex()) { + internalEnqueue(OperationQueueItem(operation.value, bucket = enqueueIntoBucket), flush = false, addToStore = false, operation.index) + } } } @@ -121,12 +128,16 @@ internal class OperationRepo( queueItem: OperationQueueItem, flush: Boolean, addToStore: Boolean, + index: Int = -1, ) { synchronized(queue) { - queue.add(queueItem) - if (addToStore) { - _operationModelStore.add(queueItem.operation) - } + if (index >= 0) + queue.add(index, queueItem) + else + queue.add(queueItem) + } + if (addToStore) { + _operationModelStore.add(queueItem.operation) } waiter.wake(LoopWaiterMessage(flush, 0)) From 0d0760d612650f7495af5ddd156c05185f8e148c Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Wed, 1 May 2024 16:05:17 -0400 Subject: [PATCH 2/5] move OperationModelStore.load() out of init block and call load after OperationRepo starts --- .../operations/impl/OperationModelStore.kt | 7 ++++- .../internal/operations/impl/OperationRepo.kt | 29 ++++++++++++------- .../internal/operations/OperationRepoTests.kt | 1 + 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt index ec90751823..30a26bd908 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt @@ -25,10 +25,15 @@ import com.onesignal.user.internal.operations.impl.executors.LoginUserOperationE import com.onesignal.user.internal.operations.impl.executors.RefreshUserOperationExecutor import com.onesignal.user.internal.operations.impl.executors.SubscriptionOperationExecutor import com.onesignal.user.internal.operations.impl.executors.UpdateUserOperationExecutor +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.launch import org.json.JSONObject internal class OperationModelStore(prefs: IPreferencesService) : ModelStore("operations", prefs) { - init { + + fun loadOperations() { load() } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 6ba00c3a1e..0c89768e26 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -81,13 +81,6 @@ internal class OperationRepo( } } this.executorsMap = executorsMap - - // load operation in a separate thread to avoid halting the main process - coroutineScope.launch { - for (operation in _operationModelStore.list().withIndex()) { - internalEnqueue(OperationQueueItem(operation.value, bucket = enqueueIntoBucket), flush = false, addToStore = false, operation.index) - } - } } override fun containsInstanceOf(type: KClass): Boolean { @@ -98,7 +91,11 @@ internal class OperationRepo( override fun start() { paused = false - coroutineScope.launch { processQueueForever() } + coroutineScope.launch { + // load saved operations first then start processing the queue to ensure correct operation order + loadSavedOperations() + processQueueForever() + } } override fun enqueue( @@ -128,10 +125,10 @@ internal class OperationRepo( queueItem: OperationQueueItem, flush: Boolean, addToStore: Boolean, - index: Int = -1, + index: Int? = null, ) { synchronized(queue) { - if (index >= 0) + if (index != null) queue.add(index, queueItem) else queue.add(queueItem) @@ -363,4 +360,16 @@ internal class OperationRepo( return ops } + + /** + * Load saved operations from preference service and add them into the queue + * NOTE: sometimes the loading might take longer than expectedly due to device's state + */ + private fun loadSavedOperations() { + // load operation in a separate thread to avoid halting the main process + _operationModelStore.loadOperations() + for (operation in _operationModelStore.list().withIndex()) { + internalEnqueue(OperationQueueItem(operation.value, bucket = enqueueIntoBucket), flush = false, addToStore = false, operation.index) + } + } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index f79d7a1a01..c5573a7419 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -35,6 +35,7 @@ private class Mocks { val operationModelStore: OperationModelStore = run { val mockOperationModelStore = mockk() + every {mockOperationModelStore.loadOperations() } just runs every { mockOperationModelStore.list() } returns listOf() every { mockOperationModelStore.add(any()) } just runs every { mockOperationModelStore.remove(any()) } just runs From bb25e1538ea2028123c6a02a639958392e3b600d Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Thu, 2 May 2024 13:19:45 -0400 Subject: [PATCH 3/5] Avoid keeping the synchronized list locked for longer than necessary --- .../onesignal/common/modeling/ModelStore.kt | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt index 5c9c7b6c67..bbbebf2c2b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt @@ -154,31 +154,34 @@ abstract class ModelStore( } protected fun load() { + if (name == null || _prefs == null) + return + + val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]") + val jsonArray = JSONArray(str) synchronized(models) { - if (name != null && _prefs != null) { - val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]") - val jsonArray = JSONArray(str) - for (index in 0 until jsonArray.length()) { - val newModel = create(jsonArray.getJSONObject(index)) ?: continue - models.add(newModel) - // listen for changes to this model - newModel.subscribe(this) - } + for (index in 0 until jsonArray.length()) { + val newModel = create(jsonArray.getJSONObject(index)) ?: continue + models.add(newModel) + // listen for changes to this model + newModel.subscribe(this) } } + } fun persist() { - synchronized(models) { - if (name != null && _prefs != null) { - val jsonArray = JSONArray() - for (model in models) { - jsonArray.put(model.toJSON()) - } + if (name == null || _prefs == null) + return - _prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, jsonArray.toString()) + val jsonArray = JSONArray() + synchronized(models) { + for (model in models) { + jsonArray.put(model.toJSON()) } } + + _prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, jsonArray.toString()) } override fun subscribe(handler: IModelStoreChangeHandler) = changeSubscription.subscribe(handler) From 92047815ccf2fd16c6104145fe22ea35ef1334b3 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Thu, 2 May 2024 15:06:50 -0400 Subject: [PATCH 4/5] Remove unused reference; Fix format and comment --- .../operations/impl/OperationModelStore.kt | 5 --- .../internal/operations/impl/OperationRepo.kt | 32 ++++++++++++------- .../internal/operations/OperationRepoTests.kt | 2 +- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt index 30a26bd908..26eaf3ac32 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt @@ -25,14 +25,9 @@ import com.onesignal.user.internal.operations.impl.executors.LoginUserOperationE import com.onesignal.user.internal.operations.impl.executors.RefreshUserOperationExecutor import com.onesignal.user.internal.operations.impl.executors.SubscriptionOperationExecutor import com.onesignal.user.internal.operations.impl.executors.UpdateUserOperationExecutor -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.GlobalScope -import kotlinx.coroutines.coroutineScope -import kotlinx.coroutines.launch import org.json.JSONObject internal class OperationModelStore(prefs: IPreferencesService) : ModelStore("operations", prefs) { - fun loadOperations() { load() } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 0c89768e26..318e3f289b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -69,10 +69,6 @@ internal class OperationRepo( private val executeBucket get() = if (enqueueIntoBucket == 0) 0 else enqueueIntoBucket - 1 - /** - * Perform initialization in background to prevent possible operation point of failures from - * blocking the creation of OperationRepo. - */ init { val executorsMap: MutableMap = mutableMapOf() for (executor in executors) { @@ -128,10 +124,11 @@ internal class OperationRepo( index: Int? = null, ) { synchronized(queue) { - if (index != null) + if (index != null) { queue.add(index, queueItem) - else + } else { queue.add(queueItem) + } } if (addToStore) { _operationModelStore.add(queueItem.operation) @@ -340,12 +337,20 @@ internal class OperationRepo( } val startingKey = - if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) startingOp.operation.createComparisonKey else startingOp.operation.modifyComparisonKey + if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) { + startingOp.operation.createComparisonKey + } else { + startingOp.operation.modifyComparisonKey + } if (queue.isNotEmpty()) { for (item in queue.toList()) { val itemKey = - if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) item.operation.createComparisonKey else item.operation.modifyComparisonKey + if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) { + item.operation.createComparisonKey + } else { + item.operation.modifyComparisonKey + } if (itemKey == "" && startingKey == "") { throw Exception("Both comparison keys can not be blank!") @@ -363,13 +368,18 @@ internal class OperationRepo( /** * Load saved operations from preference service and add them into the queue - * NOTE: sometimes the loading might take longer than expectedly due to device's state + * NOTE: Sometimes the loading might take longer than expected due to I/O reads from disk + * Any I/O implies executing time will vary greatly. */ private fun loadSavedOperations() { - // load operation in a separate thread to avoid halting the main process _operationModelStore.loadOperations() for (operation in _operationModelStore.list().withIndex()) { - internalEnqueue(OperationQueueItem(operation.value, bucket = enqueueIntoBucket), flush = false, addToStore = false, operation.index) + internalEnqueue( + OperationQueueItem(operation.value, bucket = enqueueIntoBucket), + flush = false, + addToStore = false, + operation.index, + ) } } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index c5573a7419..e3ff258b8d 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -35,7 +35,7 @@ private class Mocks { val operationModelStore: OperationModelStore = run { val mockOperationModelStore = mockk() - every {mockOperationModelStore.loadOperations() } just runs + every { mockOperationModelStore.loadOperations() } just runs every { mockOperationModelStore.list() } returns listOf() every { mockOperationModelStore.add(any()) } just runs every { mockOperationModelStore.remove(any()) } just runs From 9e364ad41e631e75f3cef9374812b756e53dc367 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Thu, 2 May 2024 15:36:32 -0400 Subject: [PATCH 5/5] Fix ktlint --- .../main/java/com/onesignal/common/modeling/ModelStore.kt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt index bbbebf2c2b..91d4b41e05 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt @@ -154,8 +154,9 @@ abstract class ModelStore( } protected fun load() { - if (name == null || _prefs == null) + if (name == null || _prefs == null) { return + } val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]") val jsonArray = JSONArray(str) @@ -167,12 +168,12 @@ abstract class ModelStore( newModel.subscribe(this) } } - } fun persist() { - if (name == null || _prefs == null) + if (name == null || _prefs == null) { return + } val jsonArray = JSONArray() synchronized(models) {