Skip to content

Commit 60bb6b1

Browse files
stainless-app[bot]stainless-app[bot]
committed
fix(client): incorrect Retry-After parsing
1 parent 607cd1a commit 60bb6b1

File tree

2 files changed

+192
-34
lines changed

2 files changed

+192
-34
lines changed

‎cas-parser-java-core/src/main/kotlin/com/cas_parser/api/core/http/RetryingHttpClient.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ private constructor(
201201
?: headers.values("Retry-After").getOrNull(0)?.let { retryAfter ->
202202
retryAfter.toFloatOrNull()?.times(TimeUnit.SECONDS.toNanos(1))
203203
?: try {
204-
ChronoUnit.MILLIS.between(
204+
ChronoUnit.NANOS.between(
205205
OffsetDateTime.now(clock),
206206
OffsetDateTime.parse(
207207
retryAfter,

‎cas-parser-java-core/src/test/kotlin/com/cas_parser/api/core/http/RetryingHttpClientTest.kt‎

Lines changed: 191 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@ import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo
2020
import com.github.tomakehurst.wiremock.junit5.WireMockTest
2121
import com.github.tomakehurst.wiremock.stubbing.Scenario
2222
import java.io.InputStream
23+
import java.time.Clock
2324
import java.time.Duration
25+
import java.time.OffsetDateTime
26+
import java.time.ZoneOffset
27+
import java.time.format.DateTimeFormatter
2428
import java.util.concurrent.CompletableFuture
2529
import org.assertj.core.api.Assertions.assertThat
2630
import org.junit.jupiter.api.BeforeEach
@@ -36,6 +40,21 @@ internal class RetryingHttpClientTest {
3640
private lateinit var baseUrl: String
3741
private lateinit var httpClient: HttpClient
3842

43+
private class RecordingSleeper : Sleeper {
44+
val durations = mutableListOf<Duration>()
45+
46+
override fun sleep(duration: Duration) {
47+
durations.add(duration)
48+
}
49+
50+
override fun sleepAsync(duration: Duration): CompletableFuture<Void> {
51+
durations.add(duration)
52+
return CompletableFuture.completedFuture(null)
53+
}
54+
55+
override fun close() {}
56+
}
57+
3958
@BeforeEach
4059
fun beforeEach(wmRuntimeInfo: WireMockRuntimeInfo) {
4160
baseUrl = wmRuntimeInfo.httpBaseUrl
@@ -86,7 +105,8 @@ internal class RetryingHttpClientTest {
86105
@ValueSource(booleans = [false, true])
87106
fun execute(async: Boolean) {
88107
stubFor(post(urlPathEqualTo("/something")).willReturn(ok()))
89-
val retryingClient = retryingHttpClientBuilder().build()
108+
val sleeper = RecordingSleeper()
109+
val retryingClient = retryingHttpClientBuilder(sleeper).build()
90110

91111
val response =
92112
retryingClient.execute(
@@ -100,6 +120,7 @@ internal class RetryingHttpClientTest {
100120

101121
assertThat(response.statusCode()).isEqualTo(200)
102122
verify(1, postRequestedFor(urlPathEqualTo("/something")))
123+
assertThat(sleeper.durations).isEmpty()
103124
assertNoResponseLeaks()
104125
}
105126

@@ -111,8 +132,12 @@ internal class RetryingHttpClientTest {
111132
.withHeader("X-Some-Header", matching("stainless-java-retry-.+"))
112133
.willReturn(ok())
113134
)
135+
val sleeper = RecordingSleeper()
114136
val retryingClient =
115-
retryingHttpClientBuilder().maxRetries(2).idempotencyHeader("X-Some-Header").build()
137+
retryingHttpClientBuilder(sleeper)
138+
.maxRetries(2)
139+
.idempotencyHeader("X-Some-Header")
140+
.build()
116141

117142
val response =
118143
retryingClient.execute(
@@ -126,20 +151,20 @@ internal class RetryingHttpClientTest {
126151

127152
assertThat(response.statusCode()).isEqualTo(200)
128153
verify(1, postRequestedFor(urlPathEqualTo("/something")))
154+
assertThat(sleeper.durations).isEmpty()
129155
assertNoResponseLeaks()
130156
}
131157

132158
@ParameterizedTest
133159
@ValueSource(booleans = [false, true])
134160
fun execute_withRetryAfterHeader(async: Boolean) {
161+
val retryAfterDate = "Wed, 21 Oct 2015 07:28:00 GMT"
135162
stubFor(
136163
post(urlPathEqualTo("/something"))
137164
// First we fail with a retry after header given as a date
138165
.inScenario("foo")
139166
.whenScenarioStateIs(Scenario.STARTED)
140-
.willReturn(
141-
serviceUnavailable().withHeader("Retry-After", "Wed, 21 Oct 2015 07:28:00 GMT")
142-
)
167+
.willReturn(serviceUnavailable().withHeader("Retry-After", retryAfterDate))
143168
.willSetStateTo("RETRY_AFTER_DATE")
144169
)
145170
stubFor(
@@ -158,7 +183,13 @@ internal class RetryingHttpClientTest {
158183
.willReturn(ok())
159184
.willSetStateTo("COMPLETED")
160185
)
161-
val retryingClient = retryingHttpClientBuilder().maxRetries(2).build()
186+
// Fix the clock to 5 seconds before the Retry-After date so the date-based backoff is
187+
// deterministic.
188+
val retryAfterDateTime =
189+
OffsetDateTime.parse(retryAfterDate, DateTimeFormatter.RFC_1123_DATE_TIME)
190+
val clock = Clock.fixed(retryAfterDateTime.minusSeconds(5).toInstant(), ZoneOffset.UTC)
191+
val sleeper = RecordingSleeper()
192+
val retryingClient = retryingHttpClientBuilder(sleeper, clock).maxRetries(2).build()
162193

163194
val response =
164195
retryingClient.execute(
@@ -186,19 +217,20 @@ internal class RetryingHttpClientTest {
186217
postRequestedFor(urlPathEqualTo("/something"))
187218
.withHeader("x-stainless-retry-count", equalTo("2")),
188219
)
220+
assertThat(sleeper.durations)
221+
.containsExactly(Duration.ofSeconds(5), Duration.ofMillis(1234))
189222
assertNoResponseLeaks()
190223
}
191224

192225
@ParameterizedTest
193226
@ValueSource(booleans = [false, true])
194227
fun execute_withOverwrittenRetryCountHeader(async: Boolean) {
228+
val retryAfterDate = "Wed, 21 Oct 2015 07:28:00 GMT"
195229
stubFor(
196230
post(urlPathEqualTo("/something"))
197231
.inScenario("foo") // first we fail with a retry after header given as a date
198232
.whenScenarioStateIs(Scenario.STARTED)
199-
.willReturn(
200-
serviceUnavailable().withHeader("Retry-After", "Wed, 21 Oct 2015 07:28:00 GMT")
201-
)
233+
.willReturn(serviceUnavailable().withHeader("Retry-After", retryAfterDate))
202234
.willSetStateTo("RETRY_AFTER_DATE")
203235
)
204236
stubFor(
@@ -208,7 +240,11 @@ internal class RetryingHttpClientTest {
208240
.willReturn(ok())
209241
.willSetStateTo("COMPLETED")
210242
)
211-
val retryingClient = retryingHttpClientBuilder().maxRetries(2).build()
243+
val retryAfterDateTime =
244+
OffsetDateTime.parse(retryAfterDate, DateTimeFormatter.RFC_1123_DATE_TIME)
245+
val clock = Clock.fixed(retryAfterDateTime.minusSeconds(5).toInstant(), ZoneOffset.UTC)
246+
val sleeper = RecordingSleeper()
247+
val retryingClient = retryingHttpClientBuilder(sleeper, clock).maxRetries(2).build()
212248

213249
val response =
214250
retryingClient.execute(
@@ -227,6 +263,7 @@ internal class RetryingHttpClientTest {
227263
postRequestedFor(urlPathEqualTo("/something"))
228264
.withHeader("x-stainless-retry-count", equalTo("42")),
229265
)
266+
assertThat(sleeper.durations).containsExactly(Duration.ofSeconds(5))
230267
assertNoResponseLeaks()
231268
}
232269

@@ -247,7 +284,8 @@ internal class RetryingHttpClientTest {
247284
.willReturn(ok())
248285
.willSetStateTo("COMPLETED")
249286
)
250-
val retryingClient = retryingHttpClientBuilder().maxRetries(1).build()
287+
val sleeper = RecordingSleeper()
288+
val retryingClient = retryingHttpClientBuilder(sleeper).maxRetries(1).build()
251289

252290
val response =
253291
retryingClient.execute(
@@ -261,6 +299,7 @@ internal class RetryingHttpClientTest {
261299

262300
assertThat(response.statusCode()).isEqualTo(200)
263301
verify(2, postRequestedFor(urlPathEqualTo("/something")))
302+
assertThat(sleeper.durations).containsExactly(Duration.ofMillis(10))
264303
assertNoResponseLeaks()
265304
}
266305

@@ -301,21 +340,12 @@ internal class RetryingHttpClientTest {
301340
override fun close() = httpClient.close()
302341
}
303342

343+
val sleeper = RecordingSleeper()
304344
val retryingClient =
305345
RetryingHttpClient.builder()
306346
.httpClient(failingHttpClient)
307347
.maxRetries(2)
308-
.sleeper(
309-
object : Sleeper {
310-
311-
override fun sleep(duration: Duration) {}
312-
313-
override fun sleepAsync(duration: Duration): CompletableFuture<Void> =
314-
CompletableFuture.completedFuture(null)
315-
316-
override fun close() {}
317-
}
318-
)
348+
.sleeper(sleeper)
319349
.build()
320350

321351
val response =
@@ -339,25 +369,153 @@ internal class RetryingHttpClientTest {
339369
postRequestedFor(urlPathEqualTo("/something"))
340370
.withHeader("x-stainless-retry-count", equalTo("0")),
341371
)
372+
// Exponential backoff with jitter: 0.5s * jitter where jitter is in [0.75, 1.0].
373+
assertThat(sleeper.durations).hasSize(1)
374+
assertThat(sleeper.durations[0]).isBetween(Duration.ofMillis(375), Duration.ofMillis(500))
342375
assertNoResponseLeaks()
343376
}
344377

345-
private fun retryingHttpClientBuilder() =
346-
RetryingHttpClient.builder()
347-
.httpClient(httpClient)
348-
// Use a no-op `Sleeper` to make the test fast.
349-
.sleeper(
350-
object : Sleeper {
378+
@ParameterizedTest
379+
@ValueSource(booleans = [false, true])
380+
fun execute_withExponentialBackoff(async: Boolean) {
381+
stubFor(post(urlPathEqualTo("/something")).willReturn(serviceUnavailable()))
382+
val sleeper = RecordingSleeper()
383+
val retryingClient = retryingHttpClientBuilder(sleeper).maxRetries(3).build()
384+
385+
val response =
386+
retryingClient.execute(
387+
HttpRequest.builder()
388+
.method(HttpMethod.POST)
389+
.baseUrl(baseUrl)
390+
.addPathSegment("something")
391+
.build(),
392+
async,
393+
)
351394

352-
override fun sleep(duration: Duration) {}
395+
// All retries exhausted; the last 503 response is returned.
396+
assertThat(response.statusCode()).isEqualTo(503)
397+
verify(4, postRequestedFor(urlPathEqualTo("/something")))
398+
// Exponential backoff with jitter: backoff = min(0.5 * 2^(retries-1), 8) * jitter where
399+
// jitter is in [0.75, 1.0].
400+
assertThat(sleeper.durations).hasSize(3)
401+
// retries=1: 0.5s * [0.75, 1.0]
402+
assertThat(sleeper.durations[0]).isBetween(Duration.ofMillis(375), Duration.ofMillis(500))
403+
// retries=2: 1.0s * [0.75, 1.0]
404+
assertThat(sleeper.durations[1]).isBetween(Duration.ofMillis(750), Duration.ofMillis(1000))
405+
// retries=3: 2.0s * [0.75, 1.0]
406+
assertThat(sleeper.durations[2]).isBetween(Duration.ofMillis(1500), Duration.ofMillis(2000))
407+
assertNoResponseLeaks()
408+
}
353409

354-
override fun sleepAsync(duration: Duration): CompletableFuture<Void> =
355-
CompletableFuture.completedFuture(null)
410+
@ParameterizedTest
411+
@ValueSource(booleans = [false, true])
412+
fun execute_withExponentialBackoffCap(async: Boolean) {
413+
stubFor(post(urlPathEqualTo("/something")).willReturn(serviceUnavailable()))
414+
val sleeper = RecordingSleeper()
415+
val retryingClient = retryingHttpClientBuilder(sleeper).maxRetries(6).build()
356416

357-
override fun close() {}
358-
}
417+
val response =
418+
retryingClient.execute(
419+
HttpRequest.builder()
420+
.method(HttpMethod.POST)
421+
.baseUrl(baseUrl)
422+
.addPathSegment("something")
423+
.build(),
424+
async,
359425
)
360426

427+
assertThat(response.statusCode()).isEqualTo(503)
428+
verify(7, postRequestedFor(urlPathEqualTo("/something")))
429+
assertThat(sleeper.durations).hasSize(6)
430+
// retries=5: min(0.5 * 2^4, 8) = 8.0s * [0.75, 1.0]
431+
assertThat(sleeper.durations[4]).isBetween(Duration.ofMillis(6000), Duration.ofMillis(8000))
432+
// retries=6: min(0.5 * 2^5, 8) = min(16, 8) = 8.0s * [0.75, 1.0] (capped)
433+
assertThat(sleeper.durations[5]).isBetween(Duration.ofMillis(6000), Duration.ofMillis(8000))
434+
assertNoResponseLeaks()
435+
}
436+
437+
@ParameterizedTest
438+
@ValueSource(booleans = [false, true])
439+
fun execute_withRetryAfterMsPriorityOverRetryAfter(async: Boolean) {
440+
stubFor(
441+
post(urlPathEqualTo("/something"))
442+
.inScenario("foo")
443+
.whenScenarioStateIs(Scenario.STARTED)
444+
.willReturn(
445+
serviceUnavailable()
446+
.withHeader("Retry-After-Ms", "50")
447+
.withHeader("Retry-After", "2")
448+
)
449+
.willSetStateTo("RETRY")
450+
)
451+
stubFor(
452+
post(urlPathEqualTo("/something"))
453+
.inScenario("foo")
454+
.whenScenarioStateIs("RETRY")
455+
.willReturn(ok())
456+
.willSetStateTo("COMPLETED")
457+
)
458+
val sleeper = RecordingSleeper()
459+
val retryingClient = retryingHttpClientBuilder(sleeper).maxRetries(1).build()
460+
461+
val response =
462+
retryingClient.execute(
463+
HttpRequest.builder()
464+
.method(HttpMethod.POST)
465+
.baseUrl(baseUrl)
466+
.addPathSegment("something")
467+
.build(),
468+
async,
469+
)
470+
471+
assertThat(response.statusCode()).isEqualTo(200)
472+
// Retry-After-Ms (50ms) takes priority over Retry-After (2s).
473+
assertThat(sleeper.durations).containsExactly(Duration.ofMillis(50))
474+
assertNoResponseLeaks()
475+
}
476+
477+
@ParameterizedTest
478+
@ValueSource(booleans = [false, true])
479+
fun execute_withRetryAfterUnparseable(async: Boolean) {
480+
stubFor(
481+
post(urlPathEqualTo("/something"))
482+
.inScenario("foo")
483+
.whenScenarioStateIs(Scenario.STARTED)
484+
.willReturn(serviceUnavailable().withHeader("Retry-After", "not-a-date-or-number"))
485+
.willSetStateTo("RETRY")
486+
)
487+
stubFor(
488+
post(urlPathEqualTo("/something"))
489+
.inScenario("foo")
490+
.whenScenarioStateIs("RETRY")
491+
.willReturn(ok())
492+
.willSetStateTo("COMPLETED")
493+
)
494+
val sleeper = RecordingSleeper()
495+
val retryingClient = retryingHttpClientBuilder(sleeper).maxRetries(1).build()
496+
497+
val response =
498+
retryingClient.execute(
499+
HttpRequest.builder()
500+
.method(HttpMethod.POST)
501+
.baseUrl(baseUrl)
502+
.addPathSegment("something")
503+
.build(),
504+
async,
505+
)
506+
507+
assertThat(response.statusCode()).isEqualTo(200)
508+
// Unparseable Retry-After falls through to exponential backoff.
509+
assertThat(sleeper.durations).hasSize(1)
510+
assertThat(sleeper.durations[0]).isBetween(Duration.ofMillis(375), Duration.ofMillis(500))
511+
assertNoResponseLeaks()
512+
}
513+
514+
private fun retryingHttpClientBuilder(
515+
sleeper: RecordingSleeper,
516+
clock: Clock = Clock.systemUTC(),
517+
) = RetryingHttpClient.builder().httpClient(httpClient).sleeper(sleeper).clock(clock)
518+
361519
private fun HttpClient.execute(request: HttpRequest, async: Boolean): HttpResponse =
362520
if (async) executeAsync(request).get() else execute(request)
363521

0 commit comments

Comments
 (0)