feat: add HTTP retry logic to update client#49
Conversation
- Add HttpRetryPolicy with exponential backoff for transient failures. - Retry on 5xx status codes, HttpRequestException, and TaskCanceledException timeouts. - Do not retry on 4xx client errors or explicit cancellation. - Integrate retries into HttpUpdateClient.DownloadStringAsync and DownloadFileAsync. - Add 7 unit tests; dotnet test passes 66/66. Closes #45.
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| HttpResponseMessage response = await operation(); | ||
| if ((int)response.StatusCode >= 500 && (int)response.StatusCode < 600) | ||
| { | ||
| response.Dispose(); | ||
| if (attempt == maxRetries) | ||
| { | ||
| throw new HttpRequestException("Server returned " + (int)response.StatusCode + " after " + maxRetries + " retries."); | ||
| } | ||
| await DelayAsync(attempt, cancellationToken); | ||
| continue; | ||
| } | ||
| return response; |
There was a problem hiding this comment.
Potential Resource Leak on Exception After Response Creation
If the operation() function returns an HttpResponseMessage, but an exception is thrown after the response is created and before it is returned (for example, in user code that wraps or processes the response), the response may not be disposed, leading to a resource leak. Consider wrapping the response handling in a try/catch/finally block to ensure the response is disposed in all failure scenarios where it is not returned to the caller.
Recommended Solution:
try {
HttpResponseMessage response = await operation();
if ((int)response.StatusCode >= 500 && (int)response.StatusCode < 600) {
response.Dispose();
// ...
}
return response;
} catch (Exception) {
// If response was created, dispose it here
throw;
}Or ensure that the operation function itself is responsible for disposing of the response on failure.
|
|
||
| public async Task DownloadFileAsync(string url, string destinationPath, CancellationToken cancellationToken = default) | ||
| { | ||
| using (HttpResponseMessage response = await _client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken)) | ||
| using (HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync( | ||
| () => _client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken), | ||
| HttpRetryPolicy.DefaultMaxRetries, | ||
| cancellationToken)) | ||
| { | ||
| response.EnsureSuccessStatusCode(); | ||
| byte[] data = await response.Content.ReadAsByteArrayAsync(); |
There was a problem hiding this comment.
Potential Unhandled IO Exceptions and Path Validation in DownloadFileAsync
The method writes the downloaded bytes directly to the specified destinationPath using File.WriteAllBytes, but does not handle IO exceptions (e.g., disk full, permission denied, invalid path) or validate the path for security (e.g., path traversal). This could result in unhandled exceptions or security vulnerabilities.
Recommended Solution:
Wrap the file write operation in a try-catch block to handle IO exceptions and validate the destination path before writing. For example:
try {
// Validate destinationPath as needed
File.WriteAllBytes(destinationPath, data);
} catch (IOException ex) {
// Handle or log the exception
throw;
}| public async Task ExecuteAsync_ThrowsOnCancellationWithoutRetry() | ||
| { | ||
| int callCount = 0; | ||
| using (CancellationTokenSource cts = new CancellationTokenSource()) | ||
| { | ||
| cts.Cancel(); | ||
| await Assert.ThrowsAsync<OperationCanceledException>(() => HttpRetryPolicy.ExecuteAsync(() => | ||
| { | ||
| callCount++; | ||
| return SuccessResponse(HttpStatusCode.OK); | ||
| }, 3, cts.Token)); | ||
| } | ||
|
|
||
| Assert.Equal(0, callCount); | ||
| } |
There was a problem hiding this comment.
The test ExecuteAsync_ThrowsOnCancellationWithoutRetry asserts that callCount is 0 after cancellation. This assumes that the cancellation token is checked before the operation is invoked. If the implementation changes to increment the counter before checking for cancellation, this test will fail even though the behavior is still correct (i.e., the operation is canceled before any work is done). To make the test more robust, consider only asserting that an OperationCanceledException is thrown, or document the expected order of operations in the implementation.
| public async Task ExecuteAsync_ThrowsAfterMaxRetriesOnServerError() | ||
| { | ||
| int callCount = 0; | ||
| await Assert.ThrowsAsync<HttpRequestException>(() => HttpRetryPolicy.ExecuteAsync(() => | ||
| { | ||
| callCount++; | ||
| return SuccessResponse(HttpStatusCode.InternalServerError); | ||
| }, 2)); | ||
|
|
||
| Assert.Equal(3, callCount); | ||
| } |
There was a problem hiding this comment.
In ExecuteAsync_ThrowsAfterMaxRetriesOnServerError, the test only asserts that an HttpRequestException is thrown and that the call count is as expected. However, it does not verify the exception message or ensure that the exception is thrown specifically due to exceeding the retry limit. To improve error detection, assert on the exception's message or a custom exception type if available, to avoid false positives from unrelated exceptions.
1 participant
Summary\r\nAdd resilient HTTP retry behavior for update checks and downloads to survive transient network or server failures.\r\n\r\n## Changes\r\n- Add with exponential backoff.\r\n- Retry on 5xx responses, , and timeout-related .\r\n- Do not retry on 4xx client errors or explicit user cancellation.\r\n- Wire retries into and .\r\n\r\n## Tests\r\n- Add 7 unit tests covering success, retry-on-5xx, no-retry-on-4xx, max-retries exhaustion, transient exception recovery, cancellation, and argument validation.\r\n- MSBUILD : error MSB1003: Specify a project or solution file. The current working directory does not contain a project or solution file. passes: 66/66.\r\n\r\nCloses #45.