From 354115eb3ac016fdba1fbd5ae5bd8c4a38859c09 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 7 May 2024 19:08:02 -0400 Subject: [PATCH 1/2] test proving loadSavedOperations index issue Test proving that loadSavedOperations can throw indexOutOfBoundsException. Real world scenario is this can happen if a few operations are added when the device is offline then the app is restarted. --- .../internal/operations/OperationRepoTests.kt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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..133e7f9108 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 @@ -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( From 8aa83c50be5a7c6a764d552986b3d305d152cbd7 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 7 May 2024 19:29:14 -0400 Subject: [PATCH 2/2] fix loadSavedOperations index out of bounds issue Since things can be added to the queue before loadSavedOperations is called it has to account for duplicate entries. It did do this, however it was missing the logic to account for not advancing the index on duplicates so an out of bounds was possible. We solved the problem by only incrementing the index if it wasn't a duplicate, however this implementation has a future landmine. If something ever removes something from the queue that loadSavedOperations added and it is still executing index problems can still happen. This scenario never happens now, but a new feature to OperationRepo or a refactor could introduce the problem. A fast follow is recommend so this landmine isn't left here. --- .../onesignal/common/modeling/ModelStore.kt | 5 ++- .../internal/operations/impl/OperationRepo.kt | 43 ++++++++++++------- .../internal/operations/OperationRepoTests.kt | 2 +- 3 files changed, 33 insertions(+), 17 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 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 133e7f9108..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()