From 387028af3d87dbe62a106994097c8f67c684a1c8 Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com> Date: Mon, 17 Oct 2022 22:17:56 +0200 Subject: [PATCH 1/8] fix: make LinearRetry behave linearly for all inputs --- gateway/src/main/kotlin/retry/LinearRetry.kt | 9 ++++--- .../src/test/kotlin/helper/LinearRetryTest.kt | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 gateway/src/test/kotlin/helper/LinearRetryTest.kt diff --git a/gateway/src/main/kotlin/retry/LinearRetry.kt b/gateway/src/main/kotlin/retry/LinearRetry.kt index 2ae114b7a32a..f5a0a67ab207 100644 --- a/gateway/src/main/kotlin/retry/LinearRetry.kt +++ b/gateway/src/main/kotlin/retry/LinearRetry.kt @@ -42,9 +42,12 @@ public class LinearRetry( override suspend fun retry() { if (!hasNext) error("max retries exceeded") - tries.incrementAndGet() - var diff = (maxBackoff - firstBackoff).inWholeMilliseconds / maxTries - diff *= tries.value + // tries/maxTries ratio * (backOffDiff) = retryProgress + val retryProgress = + ((tries.getAndIncrement() / (maxTries - 1).toDouble()) + * (maxBackoff.inWholeMilliseconds - firstBackoff.inWholeMilliseconds)).toLong() + val diff = firstBackoff.inWholeMilliseconds + retryProgress + linearRetryLogger.trace { "retry attempt ${tries.value}, delaying for $diff ms" } delay(diff) } diff --git a/gateway/src/test/kotlin/helper/LinearRetryTest.kt b/gateway/src/test/kotlin/helper/LinearRetryTest.kt new file mode 100644 index 000000000000..0c97b567949e --- /dev/null +++ b/gateway/src/test/kotlin/helper/LinearRetryTest.kt @@ -0,0 +1,24 @@ +package helper + +import dev.kord.gateway.retry.LinearRetry +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.time.Duration.Companion.milliseconds + +class LinearRetryTest { + + @Test + fun testLinearity() { + val linearRetry = LinearRetry(1.milliseconds, 10.milliseconds, 10) + val startTime = System.currentTimeMillis() + var i = 0 + runBlocking { + while (linearRetry.hasNext) { + linearRetry.retry() + i++ + } + } + assert(System.currentTimeMillis() > (startTime + 55)) + assert(i == 10) + } +} \ No newline at end of file From e097261a5b79654566663a31d42e6880210f1477 Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+toxicmushroom@users.noreply.github.com> Date: Sun, 23 Oct 2022 13:27:05 +0200 Subject: [PATCH 2/8] fix: constrain maxTries to > 1 and try to keep durations --- gateway/src/main/kotlin/retry/LinearRetry.kt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gateway/src/main/kotlin/retry/LinearRetry.kt b/gateway/src/main/kotlin/retry/LinearRetry.kt index f5a0a67ab207..a399223a9713 100644 --- a/gateway/src/main/kotlin/retry/LinearRetry.kt +++ b/gateway/src/main/kotlin/retry/LinearRetry.kt @@ -5,6 +5,7 @@ import kotlinx.atomicfu.update import kotlinx.coroutines.delay import mu.KotlinLogging import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds private val linearRetryLogger = KotlinLogging.logger { } @@ -27,7 +28,7 @@ public class LinearRetry( require( maxBackoff.minus(firstBackoff).isPositive() ) { "maxBackoff ${maxBackoff.inWholeMilliseconds} ms needs to be bigger than firstBackoff ${firstBackoff.inWholeMilliseconds} ms" } - require(maxTries > 0) { "maxTries needs to be positive but was $maxTries" } + require(maxTries > 1) { "maxTries needs to be positive but was $maxTries" } } private val tries = atomic(0) @@ -43,10 +44,10 @@ public class LinearRetry( if (!hasNext) error("max retries exceeded") // tries/maxTries ratio * (backOffDiff) = retryProgress + val ratio = tries.getAndIncrement() / (maxTries - 1).toDouble() val retryProgress = - ((tries.getAndIncrement() / (maxTries - 1).toDouble()) - * (maxBackoff.inWholeMilliseconds - firstBackoff.inWholeMilliseconds)).toLong() - val diff = firstBackoff.inWholeMilliseconds + retryProgress + (ratio * (maxBackoff - firstBackoff).inWholeMilliseconds).toLong().milliseconds + val diff = firstBackoff + retryProgress linearRetryLogger.trace { "retry attempt ${tries.value}, delaying for $diff ms" } delay(diff) From 3f6d55714c36434d96e980cef6487bac1bc5f7d9 Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+toxicmushroom@users.noreply.github.com> Date: Sun, 23 Oct 2022 19:03:49 +0200 Subject: [PATCH 3/8] fix: constrain maxTries to > 1 and try to keep durations --- gateway/src/main/kotlin/retry/LinearRetry.kt | 2 +- gateway/src/test/kotlin/helper/LinearRetryTest.kt | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/gateway/src/main/kotlin/retry/LinearRetry.kt b/gateway/src/main/kotlin/retry/LinearRetry.kt index a399223a9713..2cd7f505043b 100644 --- a/gateway/src/main/kotlin/retry/LinearRetry.kt +++ b/gateway/src/main/kotlin/retry/LinearRetry.kt @@ -49,7 +49,7 @@ public class LinearRetry( (ratio * (maxBackoff - firstBackoff).inWholeMilliseconds).toLong().milliseconds val diff = firstBackoff + retryProgress - linearRetryLogger.trace { "retry attempt ${tries.value}, delaying for $diff ms" } + linearRetryLogger.trace { "retry attempt ${tries.value}, delaying for $diff" } delay(diff) } diff --git a/gateway/src/test/kotlin/helper/LinearRetryTest.kt b/gateway/src/test/kotlin/helper/LinearRetryTest.kt index 0c97b567949e..3654814760d4 100644 --- a/gateway/src/test/kotlin/helper/LinearRetryTest.kt +++ b/gateway/src/test/kotlin/helper/LinearRetryTest.kt @@ -2,23 +2,28 @@ package helper import dev.kord.gateway.retry.LinearRetry import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest import kotlin.test.Test import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds +import kotlin.time.TimeSource +import kotlin.time.measureTime class LinearRetryTest { @Test - fun testLinearity() { - val linearRetry = LinearRetry(1.milliseconds, 10.milliseconds, 10) - val startTime = System.currentTimeMillis() + fun testLinearity() = runTest { + val linearRetry = LinearRetry(1.seconds, 10.seconds, 10) var i = 0 - runBlocking { + val elapsed = TimeSource.Monotonic.measureTime { while (linearRetry.hasNext) { linearRetry.retry() i++ } } - assert(System.currentTimeMillis() > (startTime + 55)) + + println(elapsed) + assert(elapsed >= 55.seconds) assert(i == 10) } } \ No newline at end of file From 9e39d8707fdde8622d67b3b483aaa2df1c16e810 Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com> Date: Sun, 23 Oct 2022 23:54:59 +0200 Subject: [PATCH 4/8] Add test for extreme inputs --- .../src/test/kotlin/helper/LinearRetryTest.kt | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/gateway/src/test/kotlin/helper/LinearRetryTest.kt b/gateway/src/test/kotlin/helper/LinearRetryTest.kt index 3654814760d4..89d86852278f 100644 --- a/gateway/src/test/kotlin/helper/LinearRetryTest.kt +++ b/gateway/src/test/kotlin/helper/LinearRetryTest.kt @@ -1,13 +1,11 @@ package helper import dev.kord.gateway.retry.LinearRetry -import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.currentTime import kotlinx.coroutines.test.runTest import kotlin.test.Test import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds -import kotlin.time.TimeSource -import kotlin.time.measureTime class LinearRetryTest { @@ -15,15 +13,32 @@ class LinearRetryTest { fun testLinearity() = runTest { val linearRetry = LinearRetry(1.seconds, 10.seconds, 10) var i = 0 - val elapsed = TimeSource.Monotonic.measureTime { - while (linearRetry.hasNext) { - linearRetry.retry() - i++ - } + val start = currentTime + + while (linearRetry.hasNext) { + linearRetry.retry() + i++ } - println(elapsed) + val end = currentTime + val elapsed = (end-start).milliseconds assert(elapsed >= 55.seconds) assert(i == 10) } + + @Test + fun testExtreme() = runTest { + val linearRetry = LinearRetry(1.seconds, 60.seconds, Integer.MAX_VALUE) + var i = 0 + val start = currentTime + + while (linearRetry.hasNext && i < 1000) { + linearRetry.retry() + i++ + } + + val end = currentTime + val elapsed = (end-start).milliseconds + assert(elapsed <= 1100.seconds) + } } \ No newline at end of file From 4a29295ae8cac13a6dd39e70e367ce87dacf60dc Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com> Date: Sun, 23 Oct 2022 23:55:25 +0200 Subject: [PATCH 5/8] fix: use kotlin.time multiply function --- gateway/src/main/kotlin/retry/LinearRetry.kt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gateway/src/main/kotlin/retry/LinearRetry.kt b/gateway/src/main/kotlin/retry/LinearRetry.kt index 2cd7f505043b..fef6216fb79a 100644 --- a/gateway/src/main/kotlin/retry/LinearRetry.kt +++ b/gateway/src/main/kotlin/retry/LinearRetry.kt @@ -5,7 +5,7 @@ import kotlinx.atomicfu.update import kotlinx.coroutines.delay import mu.KotlinLogging import kotlin.time.Duration -import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.times private val linearRetryLogger = KotlinLogging.logger { } @@ -45,8 +45,7 @@ public class LinearRetry( // tries/maxTries ratio * (backOffDiff) = retryProgress val ratio = tries.getAndIncrement() / (maxTries - 1).toDouble() - val retryProgress = - (ratio * (maxBackoff - firstBackoff).inWholeMilliseconds).toLong().milliseconds + val retryProgress = ratio * (maxBackoff - firstBackoff) val diff = firstBackoff + retryProgress linearRetryLogger.trace { "retry attempt ${tries.value}, delaying for $diff" } From 5f0be8db09d9b91f7df91f4b2c12760c6bb2d826 Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com> Date: Mon, 24 Oct 2022 01:23:28 +0200 Subject: [PATCH 6/8] fix: assert equality with elapsed virtual time instead of checking greater/smaller than --- gateway/src/test/kotlin/helper/LinearRetryTest.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gateway/src/test/kotlin/helper/LinearRetryTest.kt b/gateway/src/test/kotlin/helper/LinearRetryTest.kt index 89d86852278f..975fac0d388e 100644 --- a/gateway/src/test/kotlin/helper/LinearRetryTest.kt +++ b/gateway/src/test/kotlin/helper/LinearRetryTest.kt @@ -22,7 +22,8 @@ class LinearRetryTest { val end = currentTime val elapsed = (end-start).milliseconds - assert(elapsed >= 55.seconds) + + assert(elapsed == 55.seconds) assert(i == 10) } @@ -39,6 +40,7 @@ class LinearRetryTest { val end = currentTime val elapsed = (end-start).milliseconds - assert(elapsed <= 1100.seconds) + + assert(elapsed == 1000.seconds) } } \ No newline at end of file From ebe8351e8ef7724160f069e9f189e479a5dc5cce Mon Sep 17 00:00:00 2001 From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com> Date: Mon, 24 Oct 2022 01:23:48 +0200 Subject: [PATCH 7/8] fix: update error message for maxTries constraint --- gateway/src/main/kotlin/retry/LinearRetry.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateway/src/main/kotlin/retry/LinearRetry.kt b/gateway/src/main/kotlin/retry/LinearRetry.kt index fef6216fb79a..96bb00ec2a5f 100644 --- a/gateway/src/main/kotlin/retry/LinearRetry.kt +++ b/gateway/src/main/kotlin/retry/LinearRetry.kt @@ -28,7 +28,7 @@ public class LinearRetry( require( maxBackoff.minus(firstBackoff).isPositive() ) { "maxBackoff ${maxBackoff.inWholeMilliseconds} ms needs to be bigger than firstBackoff ${firstBackoff.inWholeMilliseconds} ms" } - require(maxTries > 1) { "maxTries needs to be positive but was $maxTries" } + require(maxTries > 1) { "maxTries needs to be greater than 1 but was $maxTries" } } private val tries = atomic(0) From 6c2051577d9f47cf7f21f4780a39ba3f3285a569 Mon Sep 17 00:00:00 2001 From: Lukellmann Date: Mon, 24 Oct 2022 01:45:31 +0200 Subject: [PATCH 8/8] Reformat tests --- gateway/src/test/kotlin/helper/LinearRetryTest.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gateway/src/test/kotlin/helper/LinearRetryTest.kt b/gateway/src/test/kotlin/helper/LinearRetryTest.kt index 975fac0d388e..5143c86af7b4 100644 --- a/gateway/src/test/kotlin/helper/LinearRetryTest.kt +++ b/gateway/src/test/kotlin/helper/LinearRetryTest.kt @@ -21,7 +21,7 @@ class LinearRetryTest { } val end = currentTime - val elapsed = (end-start).milliseconds + val elapsed = (end - start).milliseconds assert(elapsed == 55.seconds) assert(i == 10) @@ -29,7 +29,7 @@ class LinearRetryTest { @Test fun testExtreme() = runTest { - val linearRetry = LinearRetry(1.seconds, 60.seconds, Integer.MAX_VALUE) + val linearRetry = LinearRetry(1.seconds, 60.seconds, Int.MAX_VALUE) var i = 0 val start = currentTime @@ -39,8 +39,8 @@ class LinearRetryTest { } val end = currentTime - val elapsed = (end-start).milliseconds + val elapsed = (end - start).milliseconds assert(elapsed == 1000.seconds) } -} \ No newline at end of file +}