Skip to content

Commit

Permalink
Fix race condition while initializing EnhancedHttpRetryHelper fields
Browse files Browse the repository at this point in the history
When multiple threads try to write to the fields we can sometimes hit a case where _retryCount is set to 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
  • Loading branch information
akoeplinger committed Jul 4, 2024
1 parent 6c642c2 commit fe62a67
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 fe62a67

Please sign in to comment.