SocketHttpHandler: include host+IP information in HttpRequestException #38131
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
|
||
| [Fact] | ||
| [OuterLoop("Slow - Negative connection test")] | ||
| public async Task GetContentAsync_WhenCanNotConnect_ExceptionContainsHostInfo() |
There was a problem hiding this comment.
This should likely belong to a SocketsHttpHandler test, but I'm new to HTTP codebase, and couldn't figure out where. Any suggestion?
There was a problem hiding this comment.
I think this is a good enough place for this test.
|
|
||
| [Fact] | ||
| [OuterLoop("Slow - Negative connection test")] | ||
| public async Task GetContentAsync_WhenCanNotConnect_ExceptionContainsHostInfo() |
There was a problem hiding this comment.
I think this is a good enough place for this test.
| [OuterLoop("Slow - Negative connection test")] | ||
| public async Task GetContentAsync_WhenCanNotConnect_ExceptionContainsHostInfo() | ||
| { | ||
| using var client = new HttpClient(new SocketsHttpHandler()); |
There was a problem hiding this comment.
In tests, the HttpClient is usually created via CreateHttpClient(), e.g.: using HttpClient httpClient = CreateHttpClient();. Is there any reason here to create it directly?
There was a problem hiding this comment.
Both questions depend on what we consider to be the SUT. I can't decide what is a good functional unit to test this feature against. HttpClient with default setup or SocketsHttpHandler?
I have no preference, want to go with existing practices.
There was a problem hiding this comment.
Well, similarly looking test is here: https://github.com/dotnet/runtime/blob/master/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs#L2667-L2677
But that's a shared test code which gets executed for WinHttpHandler as well.
Maybe it would be sufficient to just add to the existing test something like:
if (!IsWinHttpHandler)
{
Assert.Contains(expected, exception.Message);
}
Note that we have many such ifs in the shared test code, so this wouldn't be a revolutionary idea.
There was a problem hiding this comment.
Since I don't think it's worth to cover all the cases HttpClientHandlerTest covers, decided to keep the test in HttpClientTest. Added CreateHttpClient() usage.
wfurt
left a comment
wfurt
left a comment
There was a problem hiding this comment.
looks generally good to me. I left few comments.
| } | ||
|
|
||
| [Fact] | ||
| [OuterLoop("Slow - Negative connection test")] |
There was a problem hiding this comment.
why this would be slow if it connects to local host? that should fail quickly, right?
There was a problem hiding this comment.
A failed connection attempt takes long on Windows. AFAIK this is because winsock will retry the connection up to 3 times even with localhost.
There was a problem hiding this comment.
how much delay do you see? The tests I was running were still pretty quick. I'm fine with leaving this outerloop.
There was a problem hiding this comment.
Failing bare Socket connections attempts take almost 2 seconds usually. This test runs even longer for some reason (about 4 seconds on my PC).
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
Outdated
Show resolved
Hide resolved
| return CancellationHelper.ShouldWrapInOperationCanceledException(error, cancellationToken) ? | ||
| CancellationHelper.CreateOperationCanceledException(error, cancellationToken) : | ||
| new HttpRequestException(error.Message, error, RequestRetryType.RetryOnNextProxy); | ||
| new HttpRequestException($"{error.Message} {host}:{port}", error, RequestRetryType.RetryOnNextProxy); |
There was a problem hiding this comment.
How this would look if host is IPv6 address? Do you know if it already comes in with the surrounding []?
There was a problem hiding this comment.
Yes, the brackets are being added here:
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Opened #38945 for the unrelated test failure. |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Fix #1326 by appending
host:portinfo toHttpRequestException's message when connection fails.Didn't change the inner
SocketException, since it would require subclassingSocketException, which would add unnecessary complexity here.