Skip to content

Commit

Permalink
Fix race condition in EnhancedHttpRetryHelper
Browse files Browse the repository at this point in the history
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 NuGet/Home#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.
  • Loading branch information
akoeplinger committed Jul 4, 2024
1 parent 6c642c2 commit 41d7bfc
Showing 1 changed file with 22 additions and 24 deletions.
46 changes: 22 additions & 24 deletions src/NuGet.Core/NuGet.Protocol/EnhancedHttpRetryHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down

0 comments on commit 41d7bfc

Please sign in to comment.