React to HttpRequestError API changes#89124
Conversation
ghost
added
the
|
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 |
| 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.
| 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.
3 participants
Fixes #89080
2bee08f and eee6bba are slight tweaks, the rest is mainly just swapping parameters around.