diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f6bc8e..7668896 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [vNext] ### Changed + +- Stop retrying HTTP 500 (Internal Server Error) and 501 (Not Implemented). Of the 5xx statuses only 502, 503 and 504 are now retried, still only for idempotent methods. A 500 is the server's own answer and asking again gets the same one, so retrying it only kept callers waiting before an error they were always going to get: in Magic Suite, behind a page loading overlay that also blocked Merlin (MS-26904). This reverses the earlier choice to retry 500; a server that returns 500 for transient faults should return 503 for them instead (#53) - Rename four builder parameters that duplicated their method name, so that they read distinctly in IntelliSense: `Key(key)` to `Key(keyValue)`, `Filter(filter)` to `Filter(filterExpression)`, `OrderBy(orderBy)` to `OrderBy(orderByExpression)` and diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs index 99bd9dd..338eaf7 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs @@ -136,11 +136,11 @@ public async Task Post_RejectedWithoutProcessing_IsStillRetried(HttpStatusCode r } /// - /// A GET still retries on 5xx exactly as before. This is the regression guard: the fix must not + /// A GET still retries on 502, 503 and 504. This is the regression guard: the fix must not /// have narrowed retries for idempotent traffic, which is the overwhelming majority. /// [Theory] - [InlineData(HttpStatusCode.InternalServerError)] + [InlineData(HttpStatusCode.ServiceUnavailable)] [InlineData(HttpStatusCode.GatewayTimeout)] public async Task Get_ServerError_IsStillRetried(HttpStatusCode serverError) { diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs index 88781f9..368fcb1 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs @@ -87,7 +87,6 @@ private static (ODataClient Client, HttpClient HttpClient, Func AttemptCoun [Theory] [InlineData(HttpStatusCode.RequestTimeout)] // 408 - the case that prompted this [InlineData(HttpStatusCode.TooManyRequests)] // 429 - [InlineData(HttpStatusCode.InternalServerError)] // 500 - must not regress [InlineData(HttpStatusCode.BadGateway)] // 502 [InlineData(HttpStatusCode.ServiceUnavailable)] // 503 [InlineData(HttpStatusCode.GatewayTimeout)] // 504 @@ -119,6 +118,8 @@ public async Task TransientStatus_IsRetried_AndSucceeds(HttpStatusCode transient [InlineData(HttpStatusCode.NotFound)] // 404 [InlineData(HttpStatusCode.Conflict)] // 409 - routine "already exists"; retrying would be wrong [InlineData(HttpStatusCode.Gone)] // 410 + [InlineData(HttpStatusCode.InternalServerError)] // 500 - the server's own answer; asking again gets the same one + [InlineData(HttpStatusCode.NotImplemented)] // 501 public async Task GenuineRejection_IsNotRetried(HttpStatusCode rejectionStatus) { var (client, httpClient, attemptCount) = CreateClient([rejectionStatus]); diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs index 6416604..2bb6111 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs @@ -8,7 +8,7 @@ namespace PanoramicData.OData.Client.Test.UnitTests; public class ODataClientRetryTests : MockedODataClientTestBase { /// - /// Tests that client retries on 500 error. + /// Tests that client retries on a 503 error. /// [Fact] public async Task Request_ServerError_Retries() @@ -21,7 +21,7 @@ public async Task Request_ServerError_Retries() callCount++; if (callCount < 3) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -137,7 +137,7 @@ public async Task Request_MaxRetries_ThrowsException() .ReturnsAsync(() => { callCount++; - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -175,7 +175,7 @@ public async Task Request_TransientFailureThatRecovers_LogsAttemptAtDebugAndNoWa callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -213,7 +213,7 @@ public async Task Request_RetriesExhaustedOnServerError_LogsSingleWarning() { // Arrange SetupSendAsync() - .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.InternalServerError) + .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }); @@ -285,7 +285,7 @@ public async Task Request_RetryAttemptLogLevelWarning_LogsAttemptsAtWarning() callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -329,7 +329,7 @@ public async Task Request_RetryAttemptLogLevelNone_LogsNoAttempts() callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -388,7 +388,7 @@ public async Task Request_RetryDelay_IsRespected() callTimes.Add(DateTime.UtcNow); if (callTimes.Count < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; diff --git a/PanoramicData.OData.Client/ODataClient.cs b/PanoramicData.OData.Client/ODataClient.cs index 515d937..12b8a9c 100644 --- a/PanoramicData.OData.Client/ODataClient.cs +++ b/PanoramicData.OData.Client/ODataClient.cs @@ -142,7 +142,7 @@ private async Task SendWithRetryAsync( /// the request without processing it - a 408 means it was never fully received, a 429 /// that it was refused outright - so repeating it cannot duplicate any effect. /// - /// 5xx is different, and only safe for an idempotent method (issue #43). A 504 in particular + /// Of the 5xx statuses only 502, 503 and 504 are retried, and only for an idempotent method (issue #43). A 504 in particular /// means the opposite of a 408: the request reached the server, the server began work, and a /// proxy gave up waiting - so the work may still be running. 502 and 503 can likewise be /// returned after an upstream has accepted a request. Retrying a POST in that state either @@ -156,7 +156,17 @@ private async Task SendWithRetryAsync( /// private static bool IsRetryableStatusCode(HttpStatusCode statusCode, HttpMethod method) => statusCode is HttpStatusCode.RequestTimeout or HttpStatusCode.TooManyRequests - || ((int)statusCode >= 500 && IsIdempotent(method)); + || (IsTransientServerError(statusCode) && IsIdempotent(method)); + + /// + /// Whether a 5xx reports an upstream that may recover (502, 503, 504) rather than the server's own answer. + /// + /// + /// A 500 or 501 is the server saying what happened, and asking again gets the same answer: retrying it + /// only keeps the caller waiting, for example behind a loading overlay, before the error it was always going to get. + /// + private static bool IsTransientServerError(HttpStatusCode statusCode) + => statusCode is HttpStatusCode.BadGateway or HttpStatusCode.ServiceUnavailable or HttpStatusCode.GatewayTimeout; /// /// Whether repeating a request with this method is guaranteed to have the same effect as making