diff --git a/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs b/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs index 635bba90232..1f623541488 100644 --- a/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs +++ b/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs @@ -1,6 +1,8 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +#nullable enable + using System; using NuGet.Common; @@ -26,6 +28,11 @@ internal class EnhancedHttpRetryHelper /// public const int DefaultRetryCount = 6; + /// + /// The default value indicating whether or not to retry HTTP 429 responses. + /// + public const bool DefaultRetry429 = true; + /// /// The environment variable used to change the delay value. /// @@ -41,6 +48,11 @@ internal class EnhancedHttpRetryHelper /// public const string RetryCountEnvironmentVariableName = "NUGET_ENHANCED_MAX_NETWORK_TRY_COUNT"; + /// + /// The environment variabled to to disable retrying HTTP 429 responses. + /// + public const string Retry429EnvironmentVariableName = "NUGET_RETRY_HTTP_429"; + private readonly IEnvironmentVariableReader _environmentVariableReader; private bool? _isEnabled = null; @@ -49,6 +61,8 @@ internal class EnhancedHttpRetryHelper private int? _delayInMilliseconds = null; + private bool? _retry429 = null; + /// /// Initializes a new instance of the class. /// @@ -74,6 +88,11 @@ public EnhancedHttpRetryHelper(IEnvironmentVariableReader environmentVariableRea /// internal int DelayInMilliseconds => _delayInMilliseconds ??= GetIntFromEnvironmentVariable(DelayInMillisecondsEnvironmentVariableName, defaultValue: DefaultDelayMilliseconds, _environmentVariableReader); + /// + /// Gets a value indicating whether or not retryable HTTP 4xx responses should be retried. + /// + internal bool Retry429 => _retry429 ??= GetBoolFromEnvironmentVariable(Retry429EnvironmentVariableName, defaultValue: true, _environmentVariableReader); + /// /// Gets a value from the specified environment variable. /// @@ -106,7 +125,7 @@ private static int GetIntFromEnvironmentVariable(string variableName, int defaul { try { - if (int.TryParse(environmentVariableReader.GetEnvironmentVariable(variableName), out int parsedValue)) + if (int.TryParse(environmentVariableReader.GetEnvironmentVariable(variableName), out int parsedValue) && parsedValue >= 0) { return parsedValue; } diff --git a/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs b/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs index 88dd9c4505c..8411e6c7fa7 100644 --- a/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs +++ b/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRetryHandler.cs @@ -204,7 +204,11 @@ public async Task SendAsync( requestUri, bodyStopwatch.ElapsedMilliseconds)); - if ((int)response.StatusCode >= 500) + int statusCode = (int)response.StatusCode; + // 5xx == server side failure + // 408 == request timeout + // 429 == too many requests + if (statusCode >= 500 || ((statusCode == 408 || statusCode == 429) && _enhancedHttpRetryHelper.Retry429)) { success = false; } diff --git a/test/NuGet.Core.Tests/NuGet.Protocol.Tests/EnhancedHttpRetryHelperTests.cs b/test/NuGet.Core.Tests/NuGet.Protocol.Tests/EnhancedHttpRetryHelperTests.cs new file mode 100644 index 00000000000..7a91437f486 --- /dev/null +++ b/test/NuGet.Core.Tests/NuGet.Protocol.Tests/EnhancedHttpRetryHelperTests.cs @@ -0,0 +1,111 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +#nullable enable + +using System.Collections.Generic; +using Test.Utility; +using Xunit; + +namespace NuGet.Protocol.Tests +{ + public class EnhancedHttpRetryHelperTests + { + [Fact] + public void NoEnvionrmentVaraiblesSet_UsesDefaultValues() + { + // Arrange + TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader(new Dictionary()); + + // Act + EnhancedHttpRetryHelper helper = new(testEnvironmentVariableReader); + + // Assert + Assert.Equal(helper.IsEnabled, EnhancedHttpRetryHelper.DefaultEnabled); + Assert.Equal(helper.RetryCount, EnhancedHttpRetryHelper.DefaultRetryCount); + Assert.Equal(helper.DelayInMilliseconds, EnhancedHttpRetryHelper.DefaultDelayMilliseconds); + Assert.Equal(helper.Retry429, EnhancedHttpRetryHelper.DefaultRetry429); + } + + [Theory] + [InlineData("")] + [InlineData("5")] + [InlineData("something")] + public void InvalidBoolValue_UsesDefault(string value) + { + // Arrange + var dict = new Dictionary() + { + [EnhancedHttpRetryHelper.IsEnabledEnvironmentVariableName] = value + }; + var environmentReader = new TestEnvironmentVariableReader(dict); + + // Act + EnhancedHttpRetryHelper helper = new(environmentReader); + + // Assert + Assert.Equal(helper.IsEnabled, EnhancedHttpRetryHelper.DefaultEnabled); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void ValidBoolValue_UsesValue(bool value) + { + // Arrange + var dict = new Dictionary() + { + [EnhancedHttpRetryHelper.IsEnabledEnvironmentVariableName] = value.ToString().ToLowerInvariant() + }; + var environmentReader = new TestEnvironmentVariableReader(dict); + + // Act + EnhancedHttpRetryHelper helper = new(environmentReader); + + // Assert + Assert.Equal(helper.IsEnabled, value); + } + + [Theory] + [InlineData("")] + [InlineData("true")] + [InlineData("something")] + [InlineData("-5")] + public void InvalidIntValue_UsesDefault(string value) + { + // Arrange + var dict = new Dictionary() + { + [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = value + }; + var environmentReader = new TestEnvironmentVariableReader(dict); + + // Act + EnhancedHttpRetryHelper helper = new(environmentReader); + + // Assert + Assert.Equal(helper.RetryCount, EnhancedHttpRetryHelper.DefaultRetryCount); + } + + [Theory] + [InlineData(5)] + [InlineData(10)] + [InlineData(100)] + public void ValidIntValue_UsesValue(int value) + { + // Arrange + var dict = new Dictionary() + { + [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = value.ToString().ToLowerInvariant() + }; + var environmentReader = new TestEnvironmentVariableReader(dict); + + // Act + EnhancedHttpRetryHelper helper = new(environmentReader); + + // Assert + Assert.Equal(helper.RetryCount, value); + } + + } +} diff --git a/test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/HttpRetryHandlerTests.cs b/test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/HttpRetryHandlerTests.cs index 76b98badae5..95795dd77aa 100644 --- a/test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/HttpRetryHandlerTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/HttpRetryHandlerTests.cs @@ -21,7 +21,7 @@ namespace NuGet.Protocol.Tests public class HttpRetryHandlerTests { private const int MaxTries = 5; - private const string TestUrl = "https://test.local/test.json"; + private const string TestUrl = "https://local.test/test.json"; private static readonly TimeSpan SmallTimeout = TimeSpan.FromMilliseconds(50); private static readonly TimeSpan LargeTimeout = TimeSpan.FromSeconds(5); @@ -100,12 +100,7 @@ public async Task HttpRetryHandler_DifferentRequestInstanceEachTime() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var requests = new HashSet(); Func handler = requestMessage => @@ -140,12 +135,7 @@ public async Task HttpRetryHandler_CancelsRequestAfterTimeout() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var requestToken = CancellationToken.None; Func> handler = async (requestMessage, token) => { @@ -182,12 +172,7 @@ public async Task HttpRetryHandler_ThrowsTimeoutExceptionForTimeout() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); Func> handler = async (requestMessage, token) => { await Task.Delay(LargeTimeout); @@ -220,12 +205,7 @@ public async Task HttpRetryHandler_MultipleTriesTimed() // Arrange TimeSpan retryDelay = SmallTimeout; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); Func handler = requestMessage => { return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable); @@ -266,12 +246,7 @@ public async Task HttpRetryHandler_MultipleTriesNoSuccess() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var hits = 0; Func handler = requestMessage => @@ -307,12 +282,7 @@ public async Task HttpRetryHandler_MultipleSuccessFirstVerifySingleHit() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var hits = 0; Func handler = requestMessage => @@ -347,12 +317,7 @@ public async Task HttpRetryHandler_404VerifySingleHit() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var hits = 0; Func handler = requestMessage => @@ -432,12 +397,7 @@ public async Task HttpRetryHandler_MultipleTriesUntilSuccess() // Arrange TimeSpan retryDelay = TimeSpan.Zero; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = MaxTries.ToString(), - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = retryDelay.TotalMilliseconds.ToString() - }); + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(); var tries = 0; var sent503 = false; @@ -501,27 +461,15 @@ public async Task HttpRetryHandler_EnhancedRetryAllowsSettingMoreRetries() } }; - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.IsEnabledEnvironmentVariableName] = bool.TrueString, - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = "11", - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = "3" - }); - - EnhancedHttpRetryHelper helper = new EnhancedHttpRetryHelper(testEnvironmentVariableReader); - Assert.Equal(helper.IsEnabled, true); - // Enhanced retry mode causes a random 0-199 ms jitter so we can't time it in this test - // but we can make sure the setting got through - Assert.Equal(helper.DelayInMilliseconds, 3); - Assert.Equal(helper.RetryCount, 11); + int retryCount = 11; + TestEnvironmentVariableReader testEnvironmentVariableReader = GetEnhancedHttpRetryEnvironmentVariables(retryCount: retryCount, delayMilliseconds: 3); var retryHandler = new HttpRetryHandler(testEnvironmentVariableReader); var testHandler = new HttpRetryTestHandler(handler); var httpClient = new HttpClient(testHandler); var request = new HttpRetryHandlerRequest(httpClient, () => new HttpRequestMessage(HttpMethod.Get, TestUrl)) { - MaxTries = helper.RetryCount, + MaxTries = retryCount, RequestTimeout = Timeout.InfiniteTimeSpan, // HttpRetryHandler will override with values from NUGET_ENHANCED_NETWORK_RETRY_DELAY_MILLISECONDS // so set this to a value that will cause test timeout if the correct value is not honored. @@ -530,7 +478,8 @@ public async Task HttpRetryHandler_EnhancedRetryAllowsSettingMoreRetries() var log = new TestLogger(); // Act - using (var response = await retryHandler.SendAsync(request, log, CancellationToken.None)) + using (CancellationTokenSource cts = new CancellationTokenSource(millisecondsDelay: 60 * 1000)) + using (var response = await retryHandler.SendAsync(request, log, cts.Token)) { // Assert Assert.True(sent503); @@ -540,28 +489,43 @@ public async Task HttpRetryHandler_EnhancedRetryAllowsSettingMoreRetries() } [Theory] - [InlineData(null, EnhancedHttpRetryHelper.DefaultEnabled)] - [InlineData("true", true)] - [InlineData("false", false)] - [InlineData("0", EnhancedHttpRetryHelper.DefaultEnabled)] - [InlineData("something", EnhancedHttpRetryHelper.DefaultEnabled)] - public void HttpRetryHandler_EnhancedRetryOnByDefault(string value, bool expectedValue) + [InlineData(true)] + [InlineData(false)] + public async Task HttpRetryHandler_429Response_RetriesWhenEnvVarSet(bool retry429) { // Arrange - TestEnvironmentVariableReader testEnvironmentVariableReader = new TestEnvironmentVariableReader( - new Dictionary() - { - [EnhancedHttpRetryHelper.IsEnabledEnvironmentVariableName] = value, - [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = null, - [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = null - }); + int attempt = 0; + // .NET Framework don't have HttpStatusCode.TooManyRequests + HttpStatusCode tooManyRequestsStatusCode = (HttpStatusCode)429; + + var busyServer = new HttpRetryTestHandler((req, cancellationToken) => + attempt++ < 2 + ? Task.FromResult(new HttpResponseMessage(tooManyRequestsStatusCode)) + : Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK))); + var client = new HttpClient(busyServer); + + var httpRetryHandlerRequest = new HttpRetryHandlerRequest(client, () => new HttpRequestMessage(HttpMethod.Get, TestUrl)); + TestEnvironmentVariableReader environmentVariables = GetEnhancedHttpRetryEnvironmentVariables(retry429: retry429); + var httpRetryHandler = new HttpRetryHandler(environmentVariables); + var logger = new TestLogger(); // Act - EnhancedHttpRetryHelper helper = new EnhancedHttpRetryHelper(testEnvironmentVariableReader); + var response = await httpRetryHandler.SendAsync(httpRetryHandlerRequest, logger, CancellationToken.None); - Assert.Equal(helper.IsEnabled, expectedValue); - Assert.Equal(helper.RetryCount, EnhancedHttpRetryHelper.DefaultRetryCount); - Assert.Equal(helper.DelayInMilliseconds, EnhancedHttpRetryHelper.DefaultDelayMilliseconds); + // Assert + if (retry429) + { + response.StatusCode.Should().Be(HttpStatusCode.OK); + attempt.Should().Be(3); + } + else + { + response.StatusCode.Should().Be(tooManyRequestsStatusCode); + attempt.Should().Be(1); + } + + string expectedMessagePrefix = " " + tooManyRequestsStatusCode.ToString() + " " + TestUrl; + logger.Messages.Where(m => m.StartsWith(expectedMessagePrefix, StringComparison.Ordinal)).Count().Should().Be(retry429 ? 2 : 1); } private static TimeSpan GetRetryMinTime(int tries, TimeSpan retryDelay) @@ -569,6 +533,16 @@ private static TimeSpan GetRetryMinTime(int tries, TimeSpan retryDelay) return TimeSpan.FromTicks((tries - 1) * retryDelay.Ticks); } + private static TestEnvironmentVariableReader GetEnhancedHttpRetryEnvironmentVariables(bool? isEnabled = true, int? retryCount = MaxTries, int? delayMilliseconds = 0, bool? retry429 = true) + => new TestEnvironmentVariableReader( + new Dictionary() + { + [EnhancedHttpRetryHelper.IsEnabledEnvironmentVariableName] = isEnabled?.ToString(), + [EnhancedHttpRetryHelper.RetryCountEnvironmentVariableName] = retryCount?.ToString(), + [EnhancedHttpRetryHelper.DelayInMillisecondsEnvironmentVariableName] = delayMilliseconds?.ToString(), + [EnhancedHttpRetryHelper.Retry429EnvironmentVariableName] = retry429?.ToString(), + }); + private class TestHandler : DelegatingHandler { public HttpRequestMessage LastRequest { get; private set; }