-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Forward StatusCode to HttpRequestException whenever possible #116257
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Tagging subscribers to this area: @dotnet/ncl |
.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs
Show resolved
Hide resolved
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. |
Are these the only 3 cases of the status code not being propagated to exceptions? |
After conducting a comprehensive search through the System.Net.Http codebase, these appear to be the only 3 cases where status codes weren't being propagated to HttpRequestException when they should have been. I systematically examined all files that throw HttpRequestException and checked for:
The other similar patterns I found are already handled correctly:
So the 3 locations identified in issue #105700 appear to be the complete set of missing cases. |
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.
Note that when I opened #105700, I was just looking at code patterns without investing time to analyze how realistic it is to hit actual practical cases of missing a status code that could be present.
This is only a good issue for copilot if we are fine fixing the code so it has the right pattern, but without looking deeper and trying to understand/test the scenarios.
.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs
Show resolved
Hide resolved
}); | ||
} | ||
catch | ||
{ |
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.
Leaving that catch block completely empty seems wrong; since this should trigger an error, shouldn't we assert that we got here?
This PR forwards HTTP status codes to
HttpRequestException
in three locations where they were previously not being included, following up on #105610 & #105546.Changes Made
HttpConnectionPool.cs (line 522): When throwing
VersionNegotiationError
due to version policy restrictions, now forwards theStatusCode
from the innerHttpRequestException
that triggered the retry.HttpConnection.cs (line 2080): When throwing
UserAuthenticationError
due to connection close during response draining, now forwards theStatusCode
from theHttpResponseMessage
.HttpConnection.cs (line 2096): When throwing
UserAuthenticationError
due to failed response draining, now forwards theStatusCode
from theHttpResponseMessage
.Example
Before this change, these exceptions would not include status code information:
After this change, status codes are properly forwarded:
This provides better diagnostic information to callers who can now access the HTTP status code that caused the underlying failure.
Testing
Fixes #105700.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
badhost
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)does.not.exist.sorry
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/delryg3w.4s2 1.1 False dns
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/3iguzv2q.i3r 1.1 True dns
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/gmfbd2cq.feu 2.0 True dns
(dns block)nosuchhost.invalid
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest+<>c <SendAsync_ExpectedDiagnosticExceptionActivityLogging>b__23_0 /tmp/bn1qqsgk.chn 2.0 True
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest+<>c <SendAsync_ExpectedDiagnosticExceptionActivityLogging>b__23_0 /tmp/3nuqi3xb.doc 1.1 False
(dns block)www.microsoft.com
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.HttpClientHandler_ServerCertificates_Test+<>c <HttpClientUsesSslCertEnvironmentVariables>b__26_0 /tmp/2tawtdzz.cvv 1.1 True
(dns block)www.some.example
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.