React to HttpRequestError API changes #89124
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/ncl |
| @@ -208,7 +208,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, lon | |||
|
|
|||
| if (quicStream == null) | |||
| { | |||
| throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure); | |||
| throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure); | |||
There was a problem hiding this comment.
Not specific to this PR, but I'm surprised some of these are "Unknown". Are we missing some kind of "UserRequested" / "Canceled" / "Aborted" / etc. enum status?
There was a problem hiding this comment.
I was considering whether some of these should be ConnectionError, but with the description of "A transport-level failure occurred while connecting to the remote endpoint.", it doesn't feel like the right fit.
A bunch of these are on retriable exceptions so in most cases the user won't see them, but it does feel like we're missing something.
| @@ -252,18 +252,18 @@ public async Task<HttpResponseMessage> SendAsync(CancellationToken cancellationT | |||
| { | |||
| case Http3ErrorCode.VersionFallback: | |||
| // The server is requesting us fall back to an older HTTP version. | |||
| throw new HttpRequestException(SR.net_http_retry_on_older_version, ex, RequestRetryType.RetryOnLowerHttpVersion); | |||
| throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_retry_on_older_version, ex, RequestRetryType.RetryOnLowerHttpVersion); | |||
There was a problem hiding this comment.
After this lands, I think it'd be worthwhile auditing everything that's still Unknown to determine whether there are additional values we should be adding to the enum.
There was a problem hiding this comment.
This is the reason why I opened #89097.
ghost
locked as resolved and limited conversation to collaborators
Fixes #89080
2bee08f and eee6bba are slight tweaks, the rest is mainly just swapping parameters around.