ConnectTimeout bubbles out imprecisely as RequestCanceled

[davidni]

Describe the bug

When HttpForwarder.SendAsync fails due to the SocketsHttpHandler hitting its configured ConnectTimeout, YARP produces result RequestCanceled. This isn't necessarily wrong, but it is imprecise and misleading (since in most other cases, RequestCanceled indicates the client deliberately aborted the request). In this case, clearly it isn't the client's fault, but rather the destination's (or something on the way to it). This caused as escalation for us due to custom code on our end producing the wrong status code as a result of the wrong expectations on the meaning of ForwarderError.RequestCanceled in this case after we started setting ConnectTimeout to a reasonable value.

Perhaps a new member in ForwarderError would be useful (sugg RequestConnectionTimedOut).

To Reproduce

Create a SocketsHttpHandler specifying ConnectTimeout = TimeSpan.FromSeconds(15), then pass it to HttpForwarder.SendAsync and attempt connecting to a really slow destination that will not accept the connection in time.

Further technical details

Notice that when SocketsHttpHandler.SendAsync fails due to a connection timeout, the resulting exception chain looks like this (as of .NET 6.0.3):

--> System.Threading.Tasks.TaskCanceledException: The operation was canceled.
--> inner exception: System.TimeoutException: A connection could not be established within the configured ConnectTimeout.

This condition is then interpreted here as ForwarderError.RequestCanceled, which isn't quite precise:

reverse-proxy/src/ReverseProxy/Forwarder/HttpForwarder.cs

Lines 464 to 471 in b6477b1

	if (!context.RequestAborted.IsCancellationRequested && requestCancellationSource.IsCancellationRequested)
	{
	return await ReportErrorAsync(ForwarderError.RequestTimedOut, StatusCodes.Status504GatewayTimeout);
	}
	else
	{
	return await ReportErrorAsync(ForwarderError.RequestCanceled, StatusCodes.Status502BadGateway);
	}

• Include the version of the packages you are using: Yarp.ReverseProxy 1.0.0 (by code inspection, latest 1.1 RC1 is also impacted)
• The platform: Windows / .NET 6.0.3

[MihaZupan]

Presumably the logic might be

if (context.RequestAborted.IsCancellationRequested)
{
    return RequestCanceled; // Client disconnect / reset
}
else if (requestCancellationSource.IsCancellationRequested)
{
    return RequestTimedOut; // Activity timeout
}
else
{
    return SomeNewTimeout; // Other sorts of timeout - which can currently only be ConnectTimeout?
}

[Tratcher]

@davidni RequestCanceled does not mean the client aborted, only that the outgoing request was canceled for some reason. It's not clear that YARP has the visibility to be able to detect anything other than RequestAborted and RequestTimedOut.

reverse-proxy/src/ReverseProxy/Forwarder/ForwarderError.cs

Lines 26 to 29 in b6477b1

	/// <summary>
	/// Canceled when trying to connect, send the request headers, or receive the response headers.
	/// </summary>
	RequestCanceled,

@MihaZupan is it possible for YARP to detect an OperationCanceledException was caused by ConnectTimeout?

[MihaZupan]

We can detect it as the situation when we observe an OCE but the token we passed to SendAsync isn't canceled.

The only reason I can think of that would cause that to happen today is the ConnectTimeout.

Edit: a custom message handler could technically throw its own exceptions.

But that assumption could potentially be invalidated in a future release (arguably we should be able to adapt if that does happen).

[davidni]

is it possible for YARP to detect an OperationCanceledException was caused by ConnectTimeout

@Tratcher / @MihaZupan perhaps this is hinting at a more fundamental issue we could tackle in SocketsHttpHandler? E.g. should the ConnectTimeout perhaps surface as an HttpRequestException? A sockets read or write timeout would surface that way, why would the Connect timeout be different?

[MihaZupan]

I don't think a timeout exception here is necessarily wrong - it is the result of ConnectTimeout.

That said, I agree this is an issue with HttpClient/SocketsHttpHandler in that it's hard to programmatically determine the root cause of the exception. I do think this is something we should improve: dotnet/runtime#67505 (comment)

[karelz]

Triage: It would be really useful for high-load scenarios to know if the endpoint failed to connect
Let's add new enum and distinguish it. Right now it is the only timeout we use, so that is fine. If we start using more timeouts, we will rely on dotnet/runtime#67505

Given it is simple and impactful, we should try to fix it in 1.1