Fix Http 1.1's read-ahead task management

[MihaZupan]

Fixes #100616

Every HTTP/1.1 connection has a ValueTask<int> _readAheadTask field that gets set either:

• from PrepareForReuse right before the call to SendAsync, or
• by the connection pool every N seconds as part of its scavenging logic that looks for dead connections to clean up (those that have received EOF)

As a result of the latter, if a connection is added to the connection pool at any point (the shared _http11Connections stack), we must assume that the read-ahead may have been set.

When "consuming" connections, we carefully manage who may consume the read-ahead task to ensure that we don't leak unobserved task exceptions.
To achieve this, we're using an int _readAheadTaskStatus flag to signal which thread will observe the result / whether the exception should be suppressed.

The problem with the current state (before this PR) is that a connection may be briefly added to the connection pool and taken out right after to handle a pending request from the request queue.
Because we know that the connection was freshly added and therefore still good, we don't bother calling into PrepareForReuse, which means we weren't updating the _readAheadTaskStatus as expected. This results in hitting a debug assert in HttpConnection.SendAsync that checks that someone claimed ownership of the read-ahead completion before sending the request (#100616).

---

Why don't we always call PrepareForReuse then?

PrepareForReuse expects its caller to immediately call into SendAsync if the check is successful. This allows it to store the stream.ReadAsync into _readAheadTask directly, without any extra wrapping logic.

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

Line 166 in 0686ce6

_readAheadTask = _stream.ReadAsync(_readBuffer.AvailableMemory);

But at this point in the connection management, we're not 100% certain that we will have a request for the connection yet - we have waiters that may get canceled at any point.
If we did call PrepareForReuse and then had no waiter to signal, we'd have to throw away the connection.

PrepareForReuse also uses different logic based on a bool async argument which we don't yet have at this point.

---

Why don't we give the connection to the waiter and have the consuming code call PrepareForReuse?

While not super likely, if the connection was already bad, it would mean that we now took a request from the head of the queue and pushed it back to the tail to retry.
It would also potentially mean needing an extra flag to remember if this is the first request on the connection to match how we currently handle new connections that are immediately bad. Without that, we could end up doing more connect retries to a faulty server than we currently do (maybe that'd be fine, but it's not an obvious consequence).

It should be a possible approach, but I found the following simpler to follow:

---

This PR:

I changed how we handle read-ahead completion ownership, allowing a thread to return its completion ownership:

• When we take a connection from the pool, we check if a scavenging read-ahead was started, and if so try to take ownership of its completion.
• If we then fail to signal a request waiter, we try to hand back the completion ownership.

If TryOwnScavengingTaskCompletion fails, the read-ahead already completed and any potential exceptions got ignored => the connection is not usable and we try again.
If TryReturnScavengingTaskCompletionOwnership fails, the read-ahead completed between us taking ownership and trying to return it => the connection is not usable and we try again.

[dotnet-policy-service]

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

[danmoseley]

Not my area but I want to recognize that the change description here is really great 😀.

[MihaZupan]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

LGTM.
Is there chance this can also be related to #31254?
It seems like there may be mix of problems but some looked similar enough to me.

[wfurt]

could still this be race condition? e.g. It did not start it but it will before we get to the next statement.

[MihaZupan]

By the time you get to PrepareForReuse, the caller is the only thing with a reference to this connection.

It's possible that the read was already started and is transitioning to Completed right under you, but it can't transition from NotStarted => something else, or from something to => NotStarted.

[MihaZupan]

Is there chance this can also be related to #31254?

I don't think so. #31254 is happening because our HTTP/1.1 logic isn't flowing the cancellation token through to the underlying stream, instead using dispose to force pending operations to abort.