Skip to content
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

SocketHttpHandler: exception is missing host/port text #1326

Closed
Suchiman opened this issue Jan 6, 2020 · 11 comments · Fixed by #38131
Closed

SocketHttpHandler: exception is missing host/port text #1326

Suchiman opened this issue Jan 6, 2020 · 11 comments · Fixed by #38131
Assignees
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@Suchiman
Copy link
Contributor

Suchiman commented Jan 6, 2020

When making HTTP requests, the inner SocketException used to print the IP Adress and Port which is quite useful for troubleshooting, e.g.

System.Net.WebException: Unable to connect to the remote server
---> System.Net.Sockets.SocketException: No connection could be made because the target machine actively refused it 127.0.0.1:80
   at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress)
   at System.Net.Sockets.Socket.InternalConnect(EndPoint remoteEP)
   at System.Net.ServicePoint.ConnectSocketInternal(Boolean connectFailure, Socket s4, Socket s6, Socket& socket, IPAddress& address, ConnectSocketState state, IAsyncResult asyncResult, Int32 timeout, Exception& exception)
   --- End of inner exception stack trace ---
   at System.Net.HttpWebRequest.GetRequestStream()

One thing you could easily spot by this was if you're getting stuck at a proxy or the real deal.
The SocketsHttpHandler does not appear to print them.

System.Net.Http.HttpRequestException: No connection could be made because the target machine actively refused it
---> System.Net.Sockets.SocketException: No connection could be made because the target machine actively refused it
   at async ValueTask<ValueTuple<Socket, Stream>> System.Net.Http.ConnectHelper.ConnectAsync(string host, int port, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at async ValueTask<ValueTuple<Socket, Stream>> System.Net.Http.ConnectHelper.ConnectAsync(string host, int port, CancellationToken cancellationToken)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> System.Net.Http.HttpConnectionPool.CreateConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> System.Net.Http.HttpConnectionPool.WaitForCreatedConnectionAsync(ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> creationTask)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async Task<HttpResponseMessage> System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, bool doRequestAuth, CancellationToken cancellationToken)
   at async ValueTask<ValueTuple<Stream, HttpResponseMessage>> System.Net.Http.HttpConnectionPool.EstablishProxyTunnel(CancellationToken cancellationToken)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> System.Net.Http.HttpConnectionPool.CreateConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> System.Net.Http.HttpConnectionPool.WaitForCreatedConnectionAsync(ValueTask<ValueTuple<HttpConnection, HttpResponseMessage>> creationTask)
   at TResult System.Threading.Tasks.ValueTask<TResult>.get_Result()
   at async Task<HttpResponseMessage> System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, bool doRequestAuth, CancellationToken cancellationToken)
   at async Task<HttpResponseMessage> System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at async Task<HttpResponseMessage> System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task<HttpResponseMessage> sendTask, HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts)
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net untriaged New issue has not been triaged by the area owner labels Jan 6, 2020
@scalablecory scalablecory added the tenet-compatibility Incompatibility with previous versions or .NET Framework label Jan 6, 2020
@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label Feb 20, 2020
@karelz karelz changed the title SocketException is missing IP / Port SocketException is missing IP / Port text Mar 5, 2020
@karelz karelz added area-System.Net.Sockets and removed untriaged New issue has not been triaged by the area owner area-System.Net labels Mar 5, 2020
@karelz karelz added this to the 5.0 milestone Mar 5, 2020
@karelz
Copy link
Member

karelz commented Mar 5, 2020

Triage: Would be really useful for (production) diagnostics - something we should improve in 5.0.

@antonfirsov
Copy link
Member

antonfirsov commented Jun 9, 2020

The issue is rooted in SocketHttpHandler's ConnectHelper:

Builder.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new SocketException((int)SocketError)));

On the synchronous path, the IP+Port information is available in locals, to get it in an asynchronous completion, we need access contextual information on SocketAsyncEventArgs. Eg, by using private field on SocketAsyncEventArgs:

internal Internals.SocketAddress? _socketAddress;

Since the field is not visible from System.Net.Http, I think the issue can only be solved by implementing asynchronous Connect methods from #33418. Those will obsolete ConnectHelper's own task creation logic.

@karelz
Copy link
Member

karelz commented Jun 16, 2020

Triage: It is ok to report just host name + port (we do not have IP available for host name resolving to multiple IPs).

@Suchiman
Copy link
Contributor Author

Suchiman commented Jun 16, 2020

@karelz
That significantly diminishes the returns though as this message now provides no information that isn't already known (or can be known easily by putting a log statement at the right position).

From my description, for example: Can i detect if i got stuck at an application visible proxy or the destination server?

we do not have IP available for host name resolving to multiple IPs

Does that mean it will attempt to connect all IP addresses in order or only use the first one? If it picks a specific one and does not attempt to connect to the other IP addresses, the choosen IP address should be printed, to allow determining if there's a particular faulty server.

@antonfirsov
Copy link
Member

antonfirsov commented Jun 16, 2020

@Suchiman when you are sending requests trough a proxy server, the "real" destination won't be transparent to you, HttpClient will connect to the proxy server instead.

With our new proposal you will observe the hostname of the proxy server in the exception message when connecting through a proxy, which should help you distinguishing between the two cases, even if it's not (always) the IP of the proxy. Does this make sense or am I missing some piece of information about your use case?

Edit
Note: if you use an IP address in your system wide proxy settings (or a custom instance for HttpClient.DefaultProxy), and IP adress will be printed to the exception message.

@Suchiman
Copy link
Contributor Author

@antonfirsov hm yes that makes sense.
This doesn't help for the second scenario though (without a proxy).

@antonfirsov
Copy link
Member

antonfirsov commented Jun 16, 2020

@Suchiman just to make sure I understand you correctly: You state that, when you are not using a proxy, it's helpful information for you to see the IP address instead of the hostname, right?

This would be great, but we we can achieve it only in future versions (post .NET 5), since we need part of #33418 to make that happen. For now, we are aiming for a solution that is good enough for most users.

As a workaround, you can use Dns.GetHostAddresses(hostName) to log IP information in your exception handling logic.

@Suchiman
Copy link
Contributor Author

Suchiman commented Jun 16, 2020

@antonfirsov yes, correct. It is useful when evaluating the logs if it is always the same address that is failing and which one it is when the hostName resolves to multiple ip addresses. Is it safe to assume that Dns.GetHostAddresses(hostName)[0] is the IPAddress that the connection tried to use?

This would be great, but we we can achieve it only in future versions (post .NET 5)

Ok, that is fine, it's not a blocker (just makes this particular scenario easier to diagnose).

@antonfirsov
Copy link
Member

Is it safe to assume that Dns.GetHostAddresses(hostName)[0] is the IPAddress that the connection tried to use?

In case of multiple IP-s, the connection algorithm will try all of them in the order returned by Dns.GetHostAddresses(hostName). If neither of the attempts is successful, the exception will include the last address, which is quite arbitrary. To me it seems like logging all addresses is actually better for diagnostics in that case.

@antonfirsov antonfirsov changed the title SocketException is missing IP / Port text SocketHttpHandler: exception is missing host/port text Jun 19, 2020
@ghost
Copy link

ghost commented Jun 19, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@antonfirsov
Copy link
Member

antonfirsov commented Jun 19, 2020

Changed the title to better reflect the details and the actual diagnostic info we are missing.

I don't think the IP info that we print in synchronous exceptions is really valuable because of the reasons we discussed in previous comments.

@Suchiman a fix for the re-scoped issue is on the way in #38131. If you think you will be still missing important diagnostic information with this fix, feel free to open a new issue!

antonfirsov added a commit that referenced this issue Jul 8, 2020
#38131)

Fix #1326 by appending host:port info to HttpRequestException's message when connection fails.

Didn't change the inner SocketException, since it would require subclassing SocketException, which would add unnecessary complexity here.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants