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 91d4b41e05..b10b3df214 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 @@ -65,8 +65,11 @@ abstract class ModelStore( } } + /** + * @return list of read-only models, cloned for thread safety + */ override fun list(): Collection { - return models + return synchronized(models) { models.toList() } } override fun get(id: String): TModel? { 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 f402233a92..d2f46e6a31 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 @@ -136,20 +136,25 @@ internal class OperationRepo( return waiter.waitForWake() } - // WARNING: Never set to true, until budget rules are added, even for internal use! + /** + * Only used inside this class, adds OperationQueueItem to queue + * WARNING: Never set flush=true until budget rules are added, even for internal use! + * + * @returns true if the OperationQueueItem was added, false if not + */ private fun internalEnqueue( queueItem: OperationQueueItem, flush: Boolean, addToStore: Boolean, index: Int? = null, - ) { - val hasExisting = queue.any { it.operation.id == queueItem.operation.id } - if (hasExisting) { - Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.") - return - } - + ): Boolean { synchronized(queue) { + val hasExisting = queue.any { it.operation.id == queueItem.operation.id } + if (hasExisting) { + Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.") + return false + } + if (index != null) { queue.add(index, queueItem) } else { @@ -161,6 +166,7 @@ internal class OperationRepo( } waiter.wake(LoopWaiterMessage(flush, 0)) + return true } /** @@ -405,18 +411,25 @@ internal class OperationRepo( /** * Load saved operations from preference service and add them into the queue + * WARNING: Make sure queue.remove is NEVER called while this method is + * running, as internalEnqueue will throw IndexOutOfBounds or put things + * out of order if what was removed was something added by this method. + * - This never happens now, but is a landmine to be aware of! * 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. */ internal fun loadSavedOperations() { _operationModelStore.loadOperations() - for (operation in _operationModelStore.list().withIndex()) { - internalEnqueue( - OperationQueueItem(operation.value, bucket = enqueueIntoBucket), - flush = false, - addToStore = false, - operation.index, - ) + var successfulIndex = 0 + for (operation in _operationModelStore.list()) { + val successful = + internalEnqueue( + OperationQueueItem(operation, bucket = enqueueIntoBucket), + flush = false, + addToStore = false, + index = successfulIndex, + ) + if (successful) successfulIndex++ } loadedSubscription.fire { it.onOperationRepoLoaded() } } 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 4976ec6436..529e794f63 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 @@ -38,7 +38,7 @@ private class Mocks { val operationStoreList = mutableListOf() val mockOperationModelStore = mockk() every { mockOperationModelStore.loadOperations() } just runs - every { mockOperationModelStore.list() } returns operationStoreList + every { mockOperationModelStore.list() } answers { operationStoreList.toList() } every { mockOperationModelStore.add(any()) } answers { operationStoreList.add(firstArg()) } every { mockOperationModelStore.remove(any()) } answers { val id = firstArg() @@ -630,6 +630,26 @@ class OperationRepoTests : FunSpec({ mocks.operationRepo.queue.size shouldBe 1 mocks.operationRepo.queue.first().operation shouldBe op } + + // Real world scenario is this can happen if a few operations are added when the device is + // offline then the app is restarted. + test("ensure loadSavedOperations doesn't index out of bounds on queue when duplicates exist") { + // Given + val mocks = Mocks() + val op1 = mockOperation() + val op2 = mockOperation() + + repeat(2) { mocks.operationModelStore.add(op1) } + mocks.operationModelStore.add(op2) + + // When + mocks.operationRepo.loadSavedOperations() + + // Then + mocks.operationRepo.queue.size shouldBe 2 + mocks.operationRepo.queue[0].operation shouldBe op1 + mocks.operationRepo.queue[1].operation shouldBe op2 + } }) { companion object { private fun mockOperation(