Re-implement cdn file upload with HttpClient #157
Conversation
📝 WalkthroughWalkthroughThe change replaces UnityWebRequest-based file upload with an asynchronous HttpClient implementation that includes retry logic with exponential backoff, improved error handling for authentication/authorization failures, and per-request cancellation timeouts. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UploadFileInternal as UploadFileInternal<br/>(Coroutine)
participant RetryAsync as UploadFileWithRetryAsync<br/>(Async)
participant HttpClient
participant ErrorHandler
Caller->>UploadFileInternal: Initiate upload
UploadFileInternal->>RetryAsync: Start async task
loop Retry Loop (max 3 attempts)
RetryAsync->>HttpClient: Send POST request
alt HTTP 2xx
HttpClient-->>RetryAsync: Success (status code)
RetryAsync-->>UploadFileInternal: Return status
else HTTP 401/403
HttpClient-->>ErrorHandler: Auth error response
ErrorHandler-->>RetryAsync: SqEditorApiAuthException
RetryAsync-->>UploadFileInternal: Throw exception
else HTTP other error
HttpClient-->>ErrorHandler: Error response
ErrorHandler-->>RetryAsync: SqEditorApiException
RetryAsync-->>UploadFileInternal: Throw exception
else Timeout
ErrorHandler-->>RetryAsync: Cancellation timeout
RetryAsync->>RetryAsync: Exponential backoff (2/4/8s)
end
end
UploadFileInternal->>Caller: Complete or OnError callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs (2)
503-517: AddIsCanceledcheck for defensive completeness.The coroutine bridge only checks
IsFaulted, but tasks can also be in aCanceledstate. WhileUploadFileWithRetryAsynccatchesTaskCanceledExceptioninternally, adding this check improves robustness against future changes.Suggested improvement
// Check for exceptions -if (uploadTask.IsFaulted) +if (uploadTask.IsFaulted || uploadTask.IsCanceled) { var ex = uploadTask.Exception?.InnerException ?? uploadTask.Exception; + if (ex == null && uploadTask.IsCanceled) + { + ex = new OperationCanceledException("Upload was canceled"); + } OnError?.Invoke(ex); yield break; }
532-535: Set an explicit Content-Type header for the upload request.The
ByteArrayContentis created without an explicit Content-Type header. Following the pattern used elsewhere in the codebase (e.g., SqEditorAppApi.cs:747, 811), explicitly set the header usingcontent.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream");or the appropriate MIME type for the uploaded file to ensure compatibility with the CDN/storage service.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs
🔇 Additional comments (2)
Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs (2)
46-54: LGTM - Good use of static HttpClient.Static
HttpClientis the recommended pattern for connection pooling and avoiding socket exhaustion. The configuration is appropriate for file upload scenarios.
584-590: Exponential backoff implementation looks correct.The backoff calculation
Math.Pow(2, attempt)yields 2s, 4s, 8s for attempts 1, 2, 3 respectively, which is a reasonable exponential backoff strategy.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (!response.IsSuccessStatusCode) | ||
| { | ||
| var responseBody = await response.Content.ReadAsStringAsync(); | ||
|
|
||
| if (response.StatusCode == System.Net.HttpStatusCode.Unauthorized || | ||
| response.StatusCode == System.Net.HttpStatusCode.Forbidden) | ||
| { | ||
| throw new SqEditorApiAuthException((int)response.StatusCode, | ||
| $"HTTP {response.StatusCode}: {response.ReasonPhrase} - {responseBody}"); | ||
| } | ||
|
|
||
| throw new SqEditorApiException( | ||
| $"HTTP {response.StatusCode}: {response.ReasonPhrase} - {responseBody}"); | ||
| } |
There was a problem hiding this comment.
Non-auth HTTP errors (4xx/5xx) will be retried, which is often undesirable for client errors.
When SqEditorApiException is thrown for non-auth HTTP errors, it's caught by the generic Exception handler at line 578 and retried. Client errors like 400 Bad Request or 404 Not Found won't succeed on retry and will waste time/resources.
Consider either:
- Not retrying
SqEditorApiException(similar to auth exceptions), or - Only retrying for 5xx server errors
Suggested fix - Don't retry SqEditorApiException
catch (SqEditorApiAuthException)
{
// Don't retry auth exceptions
throw;
}
+catch (SqEditorApiException)
+{
+ // Don't retry HTTP errors (4xx, 5xx) - these are server responses, not transient failures
+ throw;
+}
catch (Exception ex)
{
lastException = ex;
LogLine.Do($"Upload failed with exception: {ex.Message}");
}
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.