improve connection scavenge logic by doing zero-byte read

[geoffkizer]

Currently, during our periodic scavenge of idle connections, we check whether the connection is still valid (i.e. has not received EOF or invalid data) by either (a) doing a socket poll, if we have access to the underlying socket; or (b) issuing a read-ahead, when we don't have access to the underlying socket.

Approach (b) is problematic because it pins our read buffer for potentially a long period of time (on Windows), and prevents us from freeing this buffer back to the pool in the future. The negative impact of this approach can be seen here: #49431

Unfortunately, there are common cases where we don't have access to the underlying socket, and thus today have to fall back to approach (b). Most importantly, if a user supplies a ConnectCallback, then we never have access to the underlying socket, even if the ConnectCallback does trivial things like logging.

This PR addresses the problem as follows: Instead of doing a poll or a read-ahead in these cases, just do a zero-byte read. For Streams that implement proper zero-byte read support (that is, the read does not complete until data is available), this will avoid unnecessary buffer pinning. It's also probably cheaper than polling, since we only need to pend one zero-byte read in this case vs potentially multiple calls to poll.

Since we cannot depend on the Stream properly supporting zero-byte read semantics, we must issue an actual read-ahead when the zero-byte read completes. For non-compliant Streams, this results in the same behavior as previously: we will immediately pin the read buffer. For compliant Streams, the completion will only happen when either the connection is invalid (as occurs today) or when there is actual valid response data on the connection, in which case we would have issued a read-ahead when the connection is about to be reused anyway.

Note that we also issue a read-ahead when we retrieve an idle connection from the pool for reuse. This logic is essentially unchanged, but refactored a bit, and no longer shared with the scavenge logic since we want different behavior in these two cases.

Also, this PR changes the behavior in ReturnConnection (called when a connection is about to be returned to the pool) to not check for EOF or invalid data in the case where we are about to transfer the connection to a waiting request. Any invalid data would likely have been detected as part of response processing. And since we are about to reuse the connection anyway, the likelihood of detecting an invalid connection during the transfer is very small and highly timing-dependent. Note this only affects cases where a connection limit is set, since this is the only case in which we have waiting requests currently.

Related to #49431

@scalablecory @stephentoub @wfurt @dotnet/ncl

[ghost]

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

Issue Details

---

Currently, during our periodic scavenge of idle connections, we check whether the connection is still valid (i.e. has not received EOF or invalid data) by either (a) doing a socket poll, if we have access to the underlying socket; or (b) issuing a read-ahead, when we don't have access to the underlying socket.

Approach (b) is problematic because it pins our read buffer for potentially a long period of time (on Windows), and prevents us from freeing this buffer back to the pool in the future. The negative impact of this approach can be seen here: #49431

Unfortunately, there are common cases where we don't have access to the underlying socket, and thus today have to fall back to approach (b). Most importantly, if a user supplies a ConnectCallback, then we never have access to the underlying socket, even if the ConnectCallback does trivial things like logging.

This PR addresses the problem as follows: Instead of doing a poll or a read-ahead in these cases, just do a zero-byte read. For Streams that implement proper zero-byte read support (that is, the read does not complete until data is available), this will avoid unnecessary buffer pinning. It's also probably cheaper than polling, since we only need to pend one zero-byte read in this case vs potentially multiple calls to poll.

Since we cannot depend on the Stream properly supporting zero-byte read semantics, we must issue an actual read-ahead when the zero-byte read completes. For non-compliant Streams, this results in the same behavior as previously: we will immediately pin the read buffer. For compliant Streams, the completion will only happen when either the connection is invalid (as occurs today) or when there is actual valid response data on the connection, in which case we would have issued a read-ahead when the connection is about to be reused anyway.

Note that we also issue a read-ahead when we retrieve an idle connection from the pool for reuse. This logic is essentially unchanged, but refactored a bit, and no longer shared with the scavenge logic since we want different behavior in these two cases.

Also, this PR changes the behavior in ReturnConnection (called when a connection is about to be returned to the pool) to not check for EOF or invalid data in the case where we are about to transfer the connection to a waiting request. Any invalid data would likely have been detected as part of response processing. And since we are about to reuse the connection anyway, the likelihood of detecting an invalid connection during the transfer is very small and highly timing-dependent. Note this only affects cases where a connection limit is set, since this is the only case in which we have waiting requests currently.

Related to #49431

@scalablecory @stephentoub @wfurt @dotnet/ncl

Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[geoffkizer]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[stephentoub]

Some nits, but otherwise LGTM.

[stephentoub]

Nit:
Could be:

// We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
#pragma warning disable CA2012 // we're very careful to ensure the ValueTask is only consumed once, even though it's stored into a field
_readAheadTask ??= ReadAheadWithZeroByteReadAsync();
#pragma warning restore CA2012

[stephentoub]

The bodies of these if blocks are identical except for one word in the traced message. If that distinction isn't important, we could combine these:

if (connection.LifetimeExpired(nowTicks, _poolManager.Settings._pooledConnectionLifetime) ||
    !connection.PrepareForReuse(async))
{
    if (NetEventSource.Log.IsEnabled()) connection.Trace("Found expired or invalid connection in pool.");
    connection.Dispose();
    continue;
}

[geoffkizer]

The distinction seems important to me.

[geoffkizer]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).