From 41d7bfc4e8d26efa40471b3805711949b6927ede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20K=C3=B6plinger?= Date: Thu, 4 Jul 2024 12:48:10 +0200 Subject: [PATCH] Fix race condition in EnhancedHttpRetryHelper When multiple threads try to access the `RetryCount` property we sometimes hit a case where `RetryCount` returned 0 (*). This caused the loop in `ProcessHttpSourceResultAsync()` to be skipped completely because `retry <= maxRetries`: https://github.com/NuGet/NuGet.Client/blob/6c642c2d63717acd4bf92050a42691928020eb89/src/NuGet.Core/NuGet.Protocol/Utility/FindPackagesByIdNupkgDownloader.cs#L258-L328 Fix this by using `Lazy` for initializing the fields which is thread-safe. Fixes https://github.com/NuGet/Home/issues/13545 (*) The reason is that code like `int RetryCount => _retryCount ??= 6;` gets turned into: ```csharp int valueOrDefault = _retryCount.GetValueOrDefault(); if (!_retryCount.HasValue) { valueOrDefault = 6; _retryCount = valueOrDefault; return valueOrDefault; } return valueOrDefault; ``` Suppose Thread A arrives first and calls `GetValueOrDefault()` (which is 0 for int) but then Thread B interjects and sets the value, now when Thread A resumes `_retryCount.HasValue` is true so we skip the if block and return valueOrDefault i.e. 0. --- .../NuGet.Protocol/EnhancedHttpRetryHelper.cs | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs b/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs index 989c8a1f350..c1e64835bae 100644 --- a/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs +++ b/src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs @@ -70,17 +70,17 @@ internal class EnhancedHttpRetryHelper private readonly IEnvironmentVariableReader _environmentVariableReader; - private bool? _isEnabled = null; + private Lazy _isEnabled; - private int? _retryCount = null; + private Lazy _retryCount; - private int? _delayInMilliseconds = null; + private Lazy _delayInMilliseconds; - private bool? _retry429 = null; + private Lazy _retry429; - private bool? _observeRetryAfter = null; + private Lazy _observeRetryAfter; - private TimeSpan? _maxRetyAfterDelay = null; + private Lazy _maxRetyAfterDelay; /// /// Initializes a new instance of the class. @@ -90,46 +90,44 @@ internal class EnhancedHttpRetryHelper public EnhancedHttpRetryHelper(IEnvironmentVariableReader environmentVariableReader) { _environmentVariableReader = environmentVariableReader ?? throw new ArgumentNullException(nameof(environmentVariableReader)); + _isEnabled = new Lazy(() => GetBoolFromEnvironmentVariable(IsEnabledEnvironmentVariableName, defaultValue: DefaultEnabled, _environmentVariableReader)); + _retryCount = new Lazy(() => GetIntFromEnvironmentVariable(RetryCountEnvironmentVariableName, defaultValue: DefaultRetryCount, _environmentVariableReader)); + _delayInMilliseconds = new Lazy(() => GetIntFromEnvironmentVariable(DelayInMillisecondsEnvironmentVariableName, defaultValue: DefaultDelayMilliseconds, _environmentVariableReader)); + _retry429 = new Lazy(() => GetBoolFromEnvironmentVariable(Retry429EnvironmentVariableName, defaultValue: true, _environmentVariableReader)); + _observeRetryAfter = new Lazy(() => GetBoolFromEnvironmentVariable(ObserveRetryAfterEnvironmentVariableName, defaultValue: DefaultObserveRetryAfter, _environmentVariableReader)); + _maxRetyAfterDelay = new Lazy(() => + { + int maxRetryAfterDelay = GetIntFromEnvironmentVariable(MaximumRetryAfterDurationEnvironmentVariableName, defaultValue: (int)TimeSpan.FromHours(1).TotalSeconds, _environmentVariableReader); + return TimeSpan.FromSeconds(maxRetryAfterDelay); + }); } /// /// Gets a value indicating whether or not enhanced HTTP retry should be enabled. The default value is . /// - internal bool IsEnabled => _isEnabled ??= GetBoolFromEnvironmentVariable(IsEnabledEnvironmentVariableName, defaultValue: DefaultEnabled, _environmentVariableReader); + internal bool IsEnabled => _isEnabled.Value; /// /// Gets a value indicating the maximum number of times to retry. The default value is 6. /// - internal int RetryCount => _retryCount ??= GetIntFromEnvironmentVariable(RetryCountEnvironmentVariableName, defaultValue: DefaultRetryCount, _environmentVariableReader); + internal int RetryCount => _retryCount.Value; /// /// Gets a value indicating the delay in milliseconds to wait before retrying a connection. The default value is 1000. /// - internal int DelayInMilliseconds => _delayInMilliseconds ??= GetIntFromEnvironmentVariable(DelayInMillisecondsEnvironmentVariableName, defaultValue: DefaultDelayMilliseconds, _environmentVariableReader); + internal int DelayInMilliseconds => _delayInMilliseconds.Value; /// /// Gets a value indicating whether or not retryable HTTP 4xx responses should be retried. /// - internal bool Retry429 => _retry429 ??= GetBoolFromEnvironmentVariable(Retry429EnvironmentVariableName, defaultValue: true, _environmentVariableReader); + internal bool Retry429 => _retry429.Value; /// /// Gets a value indicating whether or not to observe the Retry-After header on HTTP responses. /// - internal bool ObserveRetryAfter => _observeRetryAfter ??= GetBoolFromEnvironmentVariable(ObserveRetryAfterEnvironmentVariableName, defaultValue: DefaultObserveRetryAfter, _environmentVariableReader); - - internal TimeSpan MaxRetryAfterDelay - { - get - { - if (_maxRetyAfterDelay == null) - { - int maxRetryAfterDelay = GetIntFromEnvironmentVariable(MaximumRetryAfterDurationEnvironmentVariableName, defaultValue: (int)TimeSpan.FromHours(1).TotalSeconds, _environmentVariableReader); - _maxRetyAfterDelay = TimeSpan.FromSeconds(maxRetryAfterDelay); - } + internal bool ObserveRetryAfter => _observeRetryAfter.Value; - return _maxRetyAfterDelay.Value; - } - } + internal TimeSpan MaxRetryAfterDelay => _maxRetyAfterDelay.Value; /// /// Gets a value from the specified environment variable.