Configure ping in HTTP2 #40257

aik-jahoda · 2020-08-03T08:34:16Z

Allows configure HTTP2 ping on SocketsHttpHandler:

namespace System.Net.Http
{
    public class SocketsHttpHandler
    {
        public TimeSpan KeepAlivePingDelay { get; set; }
        public TimeSpan KeepAlivePingTimeout { get; set; }
        public HttpKeepAlivePingPolicy KeepAlivePingPolicy { get; set; }
    }
    public enum HttpKeepAlivePingPolicy
    {
        WithActiveRequests,
        Always
    }
}

The KeepAlivePingDelay defines how often the ping is sent. Any incoming frame can postpone the ping request. It means during vivacious communication the ping will not be sent at all as the communication itself manifest the connection is live. The time resolution is 1s.

KeepAlivePingTimeout defines the ping ack timeout. The time resolution is 1s.

KeepAlivePingPolicy defines in what circumstances the ping is sent:

WithActiveRequests - only if the connection has an active stream
Always - send the ping always - even if the connection is returned to the connection pool and no streams are active.

When the ping detects the connection is active, it closes the connection.

What to expect

The connection is closed as soon as possible, not during Send attempt
A proxy will not close connection with active frame flow

What to not expect

Connection latency - the API don't measure connection latency

fixes #31198

--amend

Dotnet-GitSync-Bot · 2020-08-03T08:34:19Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Dotnet-GitSync-Bot · 2020-08-03T08:34:20Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/libraries/System.Net.Http.WinHttpHandler/ref/System.Net.Http.WinHttpHandler.cs

…ping

…/httpping

ManickaP

Two major questions:

Is the timer with HeartBeat right approach?
Is it worth the extra class Http2KeepAlive?

@scalablecory you're opinions?

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2KeepAlive.cs

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

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

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

scalablecory

First pass review done; will provide a 2nd pass once comments are resolved.

src/libraries/System.Net.Http/src/System/Net/Http/HttpHandlerDefaults.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2KeepAlive.cs

...ibraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2KeepAlive.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

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

ManickaP · 2020-08-04T15:32:44Z

Just thinking, but instead of the HeartBeat timer, could we just make a method with a loop with await Task.Delay(PingDelay)? Similar to ProcessIncomingFrames, i.e. started but not awaited:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L158

However, I don't know if this would be worse or better on the resources than the shared timer across all connections.

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

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

JamesNK · 2020-08-05T00:50:16Z

...ibraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

+                {
+                    thisRef.HeartBeat();
+                }
+            }, new WeakReference<HttpConnectionPoolManager>(this), TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));


All implementations I have seen of pinging says that unreasonably low values for ping delay and timeout are not allowed.

1 second for the heartbeat is fine - this is what we do in Kestrel - but I think the KeepAlivePingDelay and KeepAlivePingTimeout should disallow values smaller than a second. That way developers won't expect to be able to configure KeepAlivePingDelay = TimeSpan.FromMillisecond(1) and expect a ping to be sent every millisecond.

...ibraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2KeepAlive.cs

JamesNK · 2020-08-05T01:19:48Z

Just thinking, but instead of the HeartBeat timer, could we just make a method with a loop with await Task.Delay(PingDelay)? Similar to ProcessIncomingFrames, i.e. started but not awaited:
master/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L158

However, I don't know if this would be worse or better on the resources than the shared timer across all connections.

One problem is time would slowly drift. Each iteration would be PingDelay + time to execute heartbeat logic. You'd need to calculate that time and subtract it from PingDelay when doing the next delay. Even with that time would probably slowly drift.

I haven't looked at the Timer source but it will no doubt have optimizations that make it a better choice than Task.Delay calls in a loop.

scalablecory · 2020-08-05T01:35:06Z

Just thinking, but instead of the HeartBeat timer, could we just make a method with a loop with await Task.Delay(PingDelay)? Similar to ProcessIncomingFrames, i.e. started but not awaited:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L158

However, I don't know if this would be worse or better on the resources than the shared timer across all connections.

I haven't looked at the Timer source but it will no doubt have optimizations that make it a better choice than Task.Delay calls in a loop.

Using Timer here is a lot more efficient. Timer has no allocations once it is going. Task.Delay allocates a new timer for every call.

ManickaP · 2020-08-05T06:22:05Z

Just thinking, but instead of the HeartBeat timer, could we just make a method with a loop with await Task.Delay(PingDelay)? Similar to ProcessIncomingFrames, i.e. started but not awaited:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L158

However, I don't know if this would be worse or better on the resources than the shared timer across all connections.

I haven't looked at the Timer source but it will no doubt have optimizations that make it a better choice than Task.Delay calls in a loop.

Using Timer here is a lot more efficient. Timer has no allocations once it is going. Task.Delay allocates a new timer for every call.

Cool, it was just a suggestion. And I learned something new. Keep the heartbeat.

ManickaP · 2020-08-11T08:43:08Z