From 39a8b5223952e03ef7d6983e06f3a7ec97e8af9c Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 18 Apr 2024 11:18:39 -0400 Subject: [PATCH 1/2] Test proving OpRepo.enqueue doesn't reset waittime We want to prevent a misbehaving app stuck in a loop from continuously sending updates every opRepoExecutionInterval (5 seconds currently). By waiting for the dust to settle we ensure the app is done making updates. --- .../internal/operations/OperationRepoTests.kt | 26 +++++++++++++++++++ 1 file changed, 26 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 7a05f270ee..f79d7a1a01 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 @@ -538,6 +538,32 @@ class OperationRepoTests : FunSpec({ ) } } + + // We want to prevent a misbehaving app stuck in a loop from continuously + // sending updates every opRepoExecutionInterval (5 seconds currently). + // By waiting for the dust to settle we ensure the app is done making + // updates. + test("ensure each time enqueue is called it restarts the delay time") { + // Given + val mocks = Mocks() + mocks.configModelStore.model.opRepoExecutionInterval = 100 + + // When + mocks.operationRepo.start() + launch { + repeat(10) { + mocks.operationRepo.enqueue(mockOperation(groupComparisonType = GroupComparisonType.ALTER)) + delay(50) + } + } + val result = + withTimeoutOrNull(500) { + mocks.operationRepo.enqueueAndWait(mockOperation(groupComparisonType = GroupComparisonType.ALTER)) + } + + // Then + result shouldBe null + } }) { companion object { private fun mockOperation( From b6b60c6ab46e5d35b9d8ddc7096c4fa6415a3544 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 18 Apr 2024 11:55:29 -0400 Subject: [PATCH 2/2] restart wait time on every OperationRepo.enqueue() Every time something new is enqueued we will not restart waiting for batches timer. The motivation is to prevent a misbehaving app from continuously making network calls. We are basically saying wait for the "the dust to settle" or "the water is calm" to ensure the app is done making updates. FUTURE: Highly recommend not removing this "the dust to settle" logic, as it ensures any app stuck in a loop can't cause continuous network requests. If the delay is too long for legitimate use-cases then allow tweaking the opRepoExecutionInterval value or allow commitNow() with a budget. --- .../internal/operations/impl/OperationRepo.kt | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 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 70c09ce50f..ed3158845d 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 @@ -163,22 +163,32 @@ internal class OperationRepo( /** * Waits until a new operation is enqueued, then wait an additional * amount of time afterwards, so operations can be grouped/batched. + * NOTE: Any operations that are enqueued while waiting here causes + * the wait timer to restart over. This is intentional, we + * are basically wait for "the dust to settle" / "the water + * is calm" to ensure the app is done making updates. + * FUTURE: Highly recommend not removing this "the dust to settle" + * logic, as it ensures any app stuck in a loop can't + * cause continuous network requests. If the delay is too + * long for legitimate use-cases then allow tweaking the + * opRepoExecutionInterval value or allow commitNow() + * with a budget. */ private suspend fun waitForNewOperationAndExecutionInterval() { // 1. Wait for an operation to be enqueued var wakeMessage = waiter.waitForWake() - // 2. Wait at least the time defined in opRepoExecutionInterval - // so operations can be grouped, unless one of them used - // flush=true (AKA force) - var lastTime = _time.currentTimeMillis + // 2. Now wait opRepoExecutionInterval, restart the wait + // time everytime something new is enqueued, to ensure + // the dust has settled. var remainingTime = _configModelStore.model.opRepoExecutionInterval - wakeMessage.previousWaitedTime - while (!wakeMessage.force && remainingTime > 0) { - withTimeoutOrNull(remainingTime) { - wakeMessage = waiter.waitForWake() - } - remainingTime -= _time.currentTimeMillis - lastTime - lastTime = _time.currentTimeMillis + while (!wakeMessage.force) { + val waitedTheFullTime = + withTimeoutOrNull(remainingTime) { + wakeMessage = waiter.waitForWake() + } == null + if (waitedTheFullTime) break + remainingTime = _configModelStore.model.opRepoExecutionInterval } }