Observe any exception on HTTP connection scavenge.

[salvois]

Fixes #110951

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[MihaZupan]

Thank you for filing the issue and looking to fix it.

We don't want to unconditionally suppress the error here as it's meaningful for the request.
Instead, we'll have to figure out in what case it's possible that no thread would observe the exception.

It's not immediately obvious to me how that could happen with .NET 9.0.
Do you mind double-checking that you were indeed running on 9.0 and not 8.0?

[MihaZupan]

TransitionToCompletedAndTryOwnCompletion controls who is responsible for observing the exception.
If a thread has already flipped _readAheadTaskStatus to CompletionReserved, it's also responsible for observing any exceptions thrown by this method.
The exact exception is meaningful as the request may fail with it as the inner exception.

If it has not (task status is still Started), the method itself will observe and suppress the exception. In that case we don't necessarily care that/which exception occured, just that the read completed before a request attempted to use the connection.

[salvois]

Thank you for filing the issue and looking to fix it.

Thank you for taking time to review and being so fast in it! I drafted the PR mainly to understand how much I went off road with my interpretation. I'm still trying to create a repro under Functional.Tests but have been unsuccessful so far.

We don't want to unconditionally suppress the error here as it's meaningful for the request. Instead, we'll have to figure out in what case it's possible that no thread would observe the exception.

Long shot here: what if CheckUsabilityOnScavenge got an exception while the read-ahead task is CompletionReserved, because PrepareForReuse ran just at the "right" time, and then the connection is disposed? It looks to me nobody may be observing the read-ahead task in this case.
But I'm still biased by the hope of a "false positive" error here.

It's not immediately obvious to me how that could happen with .NET 9.0. Do you mind double-checking that you were indeed running on 9.0 and not 8.0?

I'm afraid we are: I double checked the content of our self-contained deployed package, and it contains, among the others, System.Net.Http.dll 9.0.24.52809 (2024-10-29) and runtimeconfig.json specifying Microsoft.NETCore.App and Microsoft.AspNetCore.App 9.0.0.

[MihaZupan]

If PrepareForReuse returns true, the caller is expected to always follow up with a call into HttpConnection.SendAsync, which should always observe the _readAheadTask result.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Line 123 in 756e620

/// The caller MUST call SendAsync afterwards if this method returns true, or dispose the connection if it returns false.</summary>

Either when doing the initial read here

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Lines 618 to 634 in 756e620

	ValueTask<int> vt = _readAheadTask;
	_readAheadTask = default;
	int bytesRead;
	if (vt.IsCompleted)
	{
	bytesRead = vt.Result;
	}
	else
	{
	if (NetEventSource.Log.IsEnabled() && !async)
	{
	Trace($"Pre-emptive read completed asynchronously for a synchronous request.");
	}
	bytesRead = await vt.ConfigureAwait(false);
	}

or if an exception occurs here

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Lines 854 to 859 in 756e620

	if (_readAheadTask != default)
	{
	Debug.Assert(_readAheadTaskStatus is ReadAheadTask_CompletionReserved or ReadAheadTask_Completed);
	LogExceptions(_readAheadTask.AsTask());
	}

If there's a case where we could throw between PrepareForReuse and the try block in HttpConnection.SendAsync, we would indeed leak unobserved faulted tasks, but I don't know how that'd happen.

But I'm still biased by the hope of a "false positive" error here.

It is most likely a harmless issue, but we should still track down the root cause.

[salvois]

If PrepareForReuse returns true, the caller is expected to always follow up with a call into HttpConnection.SendAsync

Makes sense to me, and confirms my understanding, thanks.
I was looking at the code path where PrepareForReuse returns false but the Exchange should indeed protect us.

What about the code paths that check for a read-ahead task not yet started, though?

In my understanding, there could be a case where both PrepareForReuse and CheckUsabilityOnScavenge see ReadAheadTaskHasStarted == false, because they set _readAheadTaskStatus later, not atomically.
If this is the case, they'd both start a read-ahead task (which doesn't sound good to me), moreover TransitionToCompletedAndTryOwnCompletion could well see ReadAheadTask_CompletionReserved as set here:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Line 162 in 756e620

_readAheadTaskStatus = ReadAheadTask_CompletionReserved;

Of course, apologies if I misunderstood the implementation!

[MihaZupan]

a case where both PrepareForReuse and CheckUsabilityOnScavenge see ReadAheadTaskHasStarted == false

While not obvious from the methods themselves, for both of those, the caller first removes the connection from the pool such that only 1 thread has access to the connection at any given point, hence why they use non-synchronized checks.

Note how both are called after popping the connection from the stack:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs

Lines 441 to 443 in 1613dd3

	while (connections.TryPop(out HttpConnection? connection))
	{
	if (connection.IsUsable(nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout))

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs

Lines 37 to 46 in 1613dd3

	while (_http11Connections.TryPop(out connection))
	{
	if (CheckExpirationOnGet(connection))
	{
	if (NetEventSource.Log.IsEnabled()) connection.Trace("Found expired HTTP/1.1 connection in pool.");
	connection.Dispose();
	continue;
	}
	if (!connection.PrepareForReuse(async))