-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Store status code in exception for ProxyTunnelError #105610
Conversation
Tagging subscribers to this area: @dotnet/ncl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so
I read through that and it doesn't actually seem like being about status, rather than the response couldn't be differentiated from the actual response, and the resulting fix was to throw this exception. Now that an exception is being thrown and that exception can carry a status code, providing that status code seems like the correct answer. It's clear it's a proxy tunnel failure from the error enum. |
that make sen se to me. Can we add test(s) and perhaps also update documentation to make it clear? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonfirsov, @wfurt, is it by-design or an accident that we didn't do this?
HttpRequestException.StatusCode
has been introduced in #32455 but hasn't been systematically forwarded for cases other than trivial EnsureSuccessStatusCode()
validations. #88974 didn't change anything in that regard.
Meaning we're likely missing the status code being included in the exception in other cases? Is there an open issue for auditing all of them? |
Opened #105700. |
Fixes #105546
Closes #66329
@antonfirsov, @wfurt, is it by-design or an accident that we didn't do this? The original PR:
https://github.com/dotnet/runtime/pull/88974/files#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R1767
has it with the status code only going into the error message but not being used to intialize the property.