From 41d7bfc4e8d26efa40471b3805711949b6927ede Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alexander=20K=C3=B6plinger?= <alex.koeplinger@outlook.com>
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<T>` 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<bool> _isEnabled;
 
-        private int? _retryCount = null;
+        private Lazy<int> _retryCount;
 
-        private int? _delayInMilliseconds = null;
+        private Lazy<int> _delayInMilliseconds;
 
-        private bool? _retry429 = null;
+        private Lazy<bool> _retry429;
 
-        private bool? _observeRetryAfter = null;
+        private Lazy<bool> _observeRetryAfter;
 
-        private TimeSpan? _maxRetyAfterDelay = null;
+        private Lazy<TimeSpan> _maxRetyAfterDelay;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="EnhancedHttpRetryHelper" /> class.
@@ -90,46 +90,44 @@ internal class EnhancedHttpRetryHelper
         public EnhancedHttpRetryHelper(IEnvironmentVariableReader environmentVariableReader)
         {
             _environmentVariableReader = environmentVariableReader ?? throw new ArgumentNullException(nameof(environmentVariableReader));
+            _isEnabled = new Lazy<bool>(() => GetBoolFromEnvironmentVariable(IsEnabledEnvironmentVariableName, defaultValue: DefaultEnabled, _environmentVariableReader));
+            _retryCount = new Lazy<int>(() => GetIntFromEnvironmentVariable(RetryCountEnvironmentVariableName, defaultValue: DefaultRetryCount, _environmentVariableReader));
+            _delayInMilliseconds = new Lazy<int>(() => GetIntFromEnvironmentVariable(DelayInMillisecondsEnvironmentVariableName, defaultValue: DefaultDelayMilliseconds, _environmentVariableReader));
+            _retry429 = new Lazy<bool>(() => GetBoolFromEnvironmentVariable(Retry429EnvironmentVariableName, defaultValue: true, _environmentVariableReader));
+            _observeRetryAfter = new Lazy<bool>(() => GetBoolFromEnvironmentVariable(ObserveRetryAfterEnvironmentVariableName, defaultValue: DefaultObserveRetryAfter, _environmentVariableReader));
+            _maxRetyAfterDelay = new Lazy<TimeSpan>(() =>
+            {
+                int maxRetryAfterDelay = GetIntFromEnvironmentVariable(MaximumRetryAfterDurationEnvironmentVariableName, defaultValue: (int)TimeSpan.FromHours(1).TotalSeconds, _environmentVariableReader);
+                return TimeSpan.FromSeconds(maxRetryAfterDelay);
+            });
         }
 
         /// <summary>
         /// Gets a value indicating whether or not enhanced HTTP retry should be enabled.  The default value is <see langword="true" />.
         /// </summary>
-        internal bool IsEnabled => _isEnabled ??= GetBoolFromEnvironmentVariable(IsEnabledEnvironmentVariableName, defaultValue: DefaultEnabled, _environmentVariableReader);
+        internal bool IsEnabled => _isEnabled.Value;
 
         /// <summary>
         /// Gets a value indicating the maximum number of times to retry.  The default value is 6.
         /// </summary>
-        internal int RetryCount => _retryCount ??= GetIntFromEnvironmentVariable(RetryCountEnvironmentVariableName, defaultValue: DefaultRetryCount, _environmentVariableReader);
+        internal int RetryCount => _retryCount.Value;
 
         /// <summary>
         /// Gets a value indicating the delay in milliseconds to wait before retrying a connection.  The default value is 1000.
         /// </summary>
-        internal int DelayInMilliseconds => _delayInMilliseconds ??= GetIntFromEnvironmentVariable(DelayInMillisecondsEnvironmentVariableName, defaultValue: DefaultDelayMilliseconds, _environmentVariableReader);
+        internal int DelayInMilliseconds => _delayInMilliseconds.Value;
 
         /// <summary>
         /// Gets a value indicating whether or not retryable HTTP 4xx responses should be retried.
         /// </summary>
-        internal bool Retry429 => _retry429 ??= GetBoolFromEnvironmentVariable(Retry429EnvironmentVariableName, defaultValue: true, _environmentVariableReader);
+        internal bool Retry429 => _retry429.Value;
 
         /// <summary>
         /// Gets a value indicating whether or not to observe the Retry-After header on HTTP responses.
         /// </summary>
-        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;
 
         /// <summary>
         /// Gets a <see cref="bool" /> value from the specified environment variable.