Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in EnhancedHttpRetryHelper #5905

Merged
merged 1 commit into from
Jul 5, 2024

Commits on Jul 4, 2024

  1. 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 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.
    akoeplinger committed Jul 4, 2024
    Configuration menu
    Copy the full SHA
    41d7bfc View commit details
    Browse the repository at this point in the history