Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct token bucket delays when experiencing system time jumps #810

Merged
merged 1 commit into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changes/9e04fd2c-136d-490b-9ecf-d6072a6f34c1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "9e04fd2c-136d-490b-9ecf-d6072a6f34c1",
"type": "bugfix",
"description": "Fix a bug where system time jumps could cause unexpected retry behavior",
"issues": [
"awslabs/smithy-kotlin#805"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,31 @@
package aws.smithy.kotlin.runtime.retries.delay

import aws.smithy.kotlin.runtime.retries.policy.RetryErrorType
import aws.smithy.kotlin.runtime.time.Clock
import aws.smithy.kotlin.runtime.time.epochMilliseconds
import kotlinx.coroutines.delay
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import kotlin.math.ceil
import kotlin.math.floor
import kotlin.math.min
import kotlin.time.ExperimentalTime
import kotlin.time.TimeSource

private const val MS_PER_S = 1_000

/**
* The standard implementation of a [RetryTokenBucket].
* @param options The configuration to use for this bucket.
* @param clock A clock to use for time calculations.
* @param timeSource A monotonic time source to use for calculating the temporal token fill of the bucket.
*/
public class StandardRetryTokenBucket(
@OptIn(ExperimentalTime::class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've gotten ourselves in trouble in the past by using experimental stdlib features (awslabs/aws-sdk-kotlin#622, awslabs/aws-sdk-kotlin#736). I don't have a better suggestion here but something to consider w.r.t any versioning policy/strategy we define.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and decided to keep usage of this type for now, despite it being marked as experimental. I've added a task aws-sdk-kotlin#860 to track revisiting all usages of experimental/unstable APIs before GA.

public class StandardRetryTokenBucket constructor(
public val options: StandardRetryTokenBucketOptions = StandardRetryTokenBucketOptions.Default,
private val clock: Clock = Clock.System,
private val timeSource: TimeSource = TimeSource.Monotonic,
) : RetryTokenBucket {
internal var capacity = options.maxCapacity
private set

private var lastTimestamp = now()
private var lastTimeMark = timeSource.markNow()
private val mutex = Mutex()

/**
Expand Down Expand Up @@ -58,13 +59,11 @@ public class StandardRetryTokenBucket(
capacity = 0
}

lastTimestamp = now()
lastTimeMark = timeSource.markNow()
}

private fun now(): Long = clock.now().epochMilliseconds

private fun refillCapacity() {
val refillMs = now() - lastTimestamp
val refillMs = lastTimeMark.elapsedNow().inWholeMilliseconds
val refillSize = floor(options.refillUnitsPerSecond.toDouble() / MS_PER_S * refillMs).toInt()
capacity = min(options.maxCapacity, capacity + refillSize)
}
Expand All @@ -73,7 +72,7 @@ public class StandardRetryTokenBucket(
refillCapacity()

capacity = min(options.maxCapacity, capacity + size)
lastTimestamp = now()
lastTimeMark = timeSource.markNow()
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
package aws.smithy.kotlin.runtime.retries.delay

import aws.smithy.kotlin.runtime.retries.policy.RetryErrorType
import aws.smithy.kotlin.runtime.time.ManualClock
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.runTest
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertIs
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.ExperimentalTime
import kotlin.time.TestTimeSource

class StandardRetryTokenBucketTest {
companion object {
Expand Down Expand Up @@ -88,17 +88,17 @@ class StandardRetryTokenBucketTest {
@OptIn(ExperimentalCoroutinesApi::class, ExperimentalTime::class)
@Test
fun testRefillOverTime() = runTest {
val clock = ManualClock()
val timeSource = TestTimeSource()

// A bucket that costs capacity for an initial try
val bucket = StandardRetryTokenBucket(DefaultOptions.copy(initialTryCost = 5), clock)
val bucket = StandardRetryTokenBucket(DefaultOptions.copy(initialTryCost = 5), timeSource)

assertEquals(10, bucket.capacity)
assertTime(0) { bucket.acquireToken() }
assertEquals(5, bucket.capacity)

// Refill rate is 10/s == 1/100ms so after 250ms we should have 2 more tokens.
clock.advance(250.milliseconds)
timeSource += 250.milliseconds

assertTime(0) { bucket.acquireToken() }
assertEquals(2, bucket.capacity) // We had 5, 2 refilled, and then we decremented 5 more.
Expand Down