From 297a94d90102ecd0e84de4744e0d3a2ecf377033 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Thu, 17 Mar 2022 18:10:34 -0700 Subject: [PATCH 1/5] Set 60 second default Retry-After when not given This is to align with the SDK guidance at https://develop.sentry.dev/sdk/rate-limiting/#rate-limit-enforcement. --- CHANGELOG.md | 4 ++++ src/Sentry/Internal/Http/RetryAfterHandler.cs | 6 ++++++ test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11ea5576e7..f6002d074a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Use a default value of 60 seconds if a `Retry-After` header is not present. ([#1443](https://github.com/getsentry/sentry-dotnet/issues/1443)) + ## 3.15.0 ### Features diff --git a/src/Sentry/Internal/Http/RetryAfterHandler.cs b/src/Sentry/Internal/Http/RetryAfterHandler.cs index a645bfc8f5..09832ad5c7 100644 --- a/src/Sentry/Internal/Http/RetryAfterHandler.cs +++ b/src/Sentry/Internal/Http/RetryAfterHandler.cs @@ -19,6 +19,7 @@ internal class RetryAfterHandler : DelegatingHandler private readonly ISystemClock _clock; private const HttpStatusCode TooManyRequests = (HttpStatusCode)429; + internal static readonly TimeSpan DefaultRetryAfterDelay = TimeSpan.FromSeconds(60); private long _retryAfterUtcTicks; internal long RetryAfterUtcTicks => _retryAfterUtcTicks; @@ -85,6 +86,11 @@ protected override async Task SendAsync( var retryAfterSpan = TimeSpan.FromSeconds(retryAfterSeconds); _ = Interlocked.Exchange(ref _retryAfterUtcTicks, _clock.GetUtcNow().AddTicks(retryAfterSpan.Ticks).UtcTicks); } + else + { + // No retry header was sent. Use the default retry delay. + _ = Interlocked.Exchange(ref _retryAfterUtcTicks, _clock.GetUtcNow().Add(DefaultRetryAfterDelay).UtcTicks); + } } return response; diff --git a/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs b/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs index de36d6fb5e..53ae2efc33 100644 --- a/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs +++ b/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs @@ -54,7 +54,7 @@ public async Task SendAsync_TooManyRequestsWithoutRetryAfterHeader_RetryAfterNot var actual = await invoker.SendAsync(new HttpRequestMessage(HttpMethod.Get, "/"), None); Assert.Equal(expected, actual); - Assert.Equal(0, _fixture.Sut.RetryAfterUtcTicks); + Assert.Equal((_fixture.TimeReturned + RetryAfterHandler.DefaultRetryAfterDelay).UtcTicks, _fixture.Sut.RetryAfterUtcTicks); Assert.True(_fixture.StubHandler.SendAsyncCalled); } From 7247e5411f0e617c66d1ae5ca747bbe0411430dd Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Thu, 17 Mar 2022 18:54:48 -0700 Subject: [PATCH 2/5] Refactor for readability --- src/Sentry/Internal/Http/RetryAfterHandler.cs | 39 +++++++++++-------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/Sentry/Internal/Http/RetryAfterHandler.cs b/src/Sentry/Internal/Http/RetryAfterHandler.cs index 09832ad5c7..dc7cd9d295 100644 --- a/src/Sentry/Internal/Http/RetryAfterHandler.cs +++ b/src/Sentry/Internal/Http/RetryAfterHandler.cs @@ -64,36 +64,43 @@ protected override async Task SendAsync( var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); - if (response.StatusCode == TooManyRequests && response.Headers != null) + if (response.StatusCode == TooManyRequests) + { + var retryAfterTimestamp = GetRetryAfterTimestamp(response); + _ = Interlocked.Exchange(ref _retryAfterUtcTicks, retryAfterTimestamp.UtcTicks); + } + + return response; + } + + private DateTimeOffset GetRetryAfterTimestamp(HttpResponseMessage response) + { + if (response.Headers != null) { if (response.Headers.RetryAfter != null) { - if (response.Headers.RetryAfter.Delta != null) + if (response.Headers.RetryAfter.Delta is { } delta) { - var retryAfterUtc = _clock.GetUtcNow() + response.Headers.RetryAfter.Delta.Value; - _ = Interlocked.Exchange(ref _retryAfterUtcTicks, retryAfterUtc.UtcTicks); + return _clock.GetUtcNow() + delta; } - else if (response.Headers.RetryAfter.Date != null) + + if (response.Headers.RetryAfter.Date is { } date) { - _ = Interlocked.Exchange(ref _retryAfterUtcTicks, response.Headers.RetryAfter.Date.Value.UtcTicks); + return date; } } + // Sentry was sending floating point numbers which are not handled by RetryConditionHeaderValue // To be compatible with older versions of sentry on premise: https://github.com/getsentry/sentry/issues/7919 - else if (response.Headers.TryGetValues("Retry-After", out var values) - && double.TryParse(values?.FirstOrDefault(), out var retryAfterSeconds)) + if (response.Headers.TryGetValues("Retry-After", out var values) + && double.TryParse(values?.FirstOrDefault(), out var retryAfterSeconds)) { - var retryAfterSpan = TimeSpan.FromSeconds(retryAfterSeconds); - _ = Interlocked.Exchange(ref _retryAfterUtcTicks, _clock.GetUtcNow().AddTicks(retryAfterSpan.Ticks).UtcTicks); - } - else - { - // No retry header was sent. Use the default retry delay. - _ = Interlocked.Exchange(ref _retryAfterUtcTicks, _clock.GetUtcNow().Add(DefaultRetryAfterDelay).UtcTicks); + return _clock.GetUtcNow() + TimeSpan.FromSeconds(retryAfterSeconds); } } - return response; + // No retry header was sent. Use the default retry delay. + return _clock.GetUtcNow() + DefaultRetryAfterDelay; } } } From 661c79b51b6134328508d11f6b1a904117507d21 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Thu, 17 Mar 2022 19:05:24 -0700 Subject: [PATCH 3/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6002d074a..2d03942e6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Use a default value of 60 seconds if a `Retry-After` header is not present. ([#1443](https://github.com/getsentry/sentry-dotnet/issues/1443)) +- Use a default value of 60 seconds if a `Retry-After` header is not present. ([#1537](https://github.com/getsentry/sentry-dotnet/pull/1537)) ## 3.15.0 From 16f91c25371341aea62298ff2e6b5f83c090f84b Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Thu, 17 Mar 2022 23:19:14 -0700 Subject: [PATCH 4/5] Use full double precision See dotnet/runtime#66815 --- src/Sentry/Internal/Http/RetryAfterHandler.cs | 2 +- test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Internal/Http/RetryAfterHandler.cs b/src/Sentry/Internal/Http/RetryAfterHandler.cs index dc7cd9d295..a6bf84c6e1 100644 --- a/src/Sentry/Internal/Http/RetryAfterHandler.cs +++ b/src/Sentry/Internal/Http/RetryAfterHandler.cs @@ -95,7 +95,7 @@ private DateTimeOffset GetRetryAfterTimestamp(HttpResponseMessage response) if (response.Headers.TryGetValues("Retry-After", out var values) && double.TryParse(values?.FirstOrDefault(), out var retryAfterSeconds)) { - return _clock.GetUtcNow() + TimeSpan.FromSeconds(retryAfterSeconds); + return _clock.GetUtcNow().AddTicks((long)(retryAfterSeconds * TimeSpan.TicksPerSecond)); } } diff --git a/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs b/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs index 53ae2efc33..3b4f569fe6 100644 --- a/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs +++ b/test/Sentry.Tests/Internals/Http/RetryAfterHandlerTests.cs @@ -104,7 +104,8 @@ public async Task SendAsync_TooManyRequestsWithRetryAfterHeaderFloat_RetryAfterS var actual = await invoker.SendAsync(new HttpRequestMessage(HttpMethod.Get, "/"), None); Assert.Equal(expected, actual); - Assert.Equal((_fixture.TimeReturned + TimeSpan.FromSeconds(floating)).UtcTicks, _fixture.Sut.RetryAfterUtcTicks); + var expectedTime = _fixture.TimeReturned.AddTicks((long)(floating * TimeSpan.TicksPerSecond)); + Assert.Equal(expectedTime.UtcTicks, _fixture.Sut.RetryAfterUtcTicks); Assert.True(_fixture.StubHandler.SendAsyncCalled); } @@ -127,7 +128,8 @@ public async Task SendAsync_TooManyRequestsWithRetryAfterHeaderFloat_SecondReque var actual = await invoker.SendAsync(new HttpRequestMessage(HttpMethod.Get, "/"), None); Assert.Equal(TooManyRequests, actual.StatusCode); - Assert.Equal((_fixture.TimeReturned + TimeSpan.FromSeconds(floating)).UtcTicks, _fixture.Sut.RetryAfterUtcTicks); + var expectedTime = _fixture.TimeReturned.AddTicks((long)(floating * TimeSpan.TicksPerSecond)); + Assert.Equal(expectedTime.UtcTicks, _fixture.Sut.RetryAfterUtcTicks); Assert.False(_fixture.StubHandler.SendAsyncCalled); } From 8d599d71a929f843b9161a3d35d745661bfd49b3 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Mon, 21 Mar 2022 07:44:10 -0700 Subject: [PATCH 5/5] Cleanup --- src/Sentry/Internal/Http/RetryAfterHandler.cs | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/Sentry/Internal/Http/RetryAfterHandler.cs b/src/Sentry/Internal/Http/RetryAfterHandler.cs index a6bf84c6e1..2682476088 100644 --- a/src/Sentry/Internal/Http/RetryAfterHandler.cs +++ b/src/Sentry/Internal/Http/RetryAfterHandler.cs @@ -75,30 +75,27 @@ protected override async Task SendAsync( private DateTimeOffset GetRetryAfterTimestamp(HttpResponseMessage response) { - if (response.Headers != null) + if (response.Headers.RetryAfter != null) { - if (response.Headers.RetryAfter != null) + if (response.Headers.RetryAfter.Delta is { } delta) { - if (response.Headers.RetryAfter.Delta is { } delta) - { - return _clock.GetUtcNow() + delta; - } - - if (response.Headers.RetryAfter.Date is { } date) - { - return date; - } + return _clock.GetUtcNow() + delta; } - // Sentry was sending floating point numbers which are not handled by RetryConditionHeaderValue - // To be compatible with older versions of sentry on premise: https://github.com/getsentry/sentry/issues/7919 - if (response.Headers.TryGetValues("Retry-After", out var values) - && double.TryParse(values?.FirstOrDefault(), out var retryAfterSeconds)) + if (response.Headers.RetryAfter.Date is { } date) { - return _clock.GetUtcNow().AddTicks((long)(retryAfterSeconds * TimeSpan.TicksPerSecond)); + return date; } } + // Sentry was sending floating point numbers which are not handled by RetryConditionHeaderValue + // To be compatible with older versions of sentry on premise: https://github.com/getsentry/sentry/issues/7919 + if (response.Headers.TryGetValues("Retry-After", out var values) + && double.TryParse(values.FirstOrDefault(), out var retryAfterSeconds)) + { + return _clock.GetUtcNow().AddTicks((long)(retryAfterSeconds * TimeSpan.TicksPerSecond)); + } + // No retry header was sent. Use the default retry delay. return _clock.GetUtcNow() + DefaultRetryAfterDelay; }