Implement #1156 by having the server send Ping messages #1161

analogrelay · 2017-11-27T22:02:58Z

This change teaches HubEndpoint to send PingMessage.Instance to the client at regular intervals, if no other messages have been sent. But only for SSE and WebSockets.

I found a sneaky and possibly ingenious (or possibly terrible) way to implement keep-alive. Since the HubEndpoint already creates a Channel that it uses to marshal writes from multiple writers, I just wrap it in a special ChannelReader that emits PingMessage.Instance if WaitToReadAsync blocks for more than the desired interval.

There is a feature, IConnectionInherentKeepAliveFeature, that indicates if a transport has "inherent keep-alive", like Long Polling (for which the poll itself serves as a way for the client to tell if the server is gone)

Notes:

package-lock.json got changed, but I sense it will keep changing if we don't check in the changes, they're just ordering changes, no new packages.
The Redis Docker tests are flaky on my home desktop (seems to be more than just Flaky Docker Redis tests #1160 after all @BrennanConroy) so I had to disable them

Fixes #1156

Also fixes #715

BrennanConroy · 2017-11-27T22:11:57Z

samples/SocketsSample/wwwroot/hubs.html

    disconnectButton.disabled = false;
    console.log('http://' + document.location.host + '/' + hubRoute);
-    connection = new signalR.HubConnection(hubRoute, logger, { transport: transportType, logger: logger });
+    connection = new signalR.HubConnection(hubRoute, { transport: transportType, logging: logger });


Copycat https://github.com/aspnet/SignalR/pull/1158/files#diff-c80b6b1153b8f1588d86994e37bb3c7fR107

Mine's better, I fixed all of them :P.

BrennanConroy · 2017-11-27T22:17:18Z

src/Microsoft.AspNetCore.Sockets.Http/Internal/Transports/LongPollingTransport.cs

 using System.Threading.Channels;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Logging;
+using Microsoft.AspNetCore.Sockets.Features;


BrennanConroy · 2017-11-27T22:48:18Z

src/Microsoft.AspNetCore.Sockets.Http/HttpSocketOptions.cs

        public LongPollingOptions LongPolling { get; } = new LongPollingOptions();
+
+        /// <summary>
+        /// The interval at which keep-alive messages should be sent. This will be 


Exclamation point at the end This will be!

davidfowl · 2017-11-28T08:59:20Z

src/Microsoft.AspNetCore.SignalR.Core/Internal/KeepAliveChannel.cs

+
+namespace Microsoft.AspNetCore.SignalR.Core.Internal
+{
+    internal class KeepAliveChannel : Channel<HubMessage>


I'm not too sure about this..

The thing I like about it is that it keeps the consumption code clean. Ideally, you really want to put keep-alive code in the HubEndPoint.OnConnectionAsync.WriteToTransport local function, since that's the central place messages appear. We could have a background task firing pings, but the goal of the ping is to send one if the timeout elapses while waiting for a message. A fixed interval kinda achieves that, but in a weird way and requires synchronization in order to know the time since the last message.

I tried putting this logic into the consumption side (in WriteToTransport) and it gets complicated fast, because KeepAlive isn't always enabled. Though I have another idea I'm going to try.

This class as a concept or name??

Probably the concept, it's a little sketchy and I'm not super happy with it myself.

analogrelay · 2017-11-28T16:40:42Z

🆙 📅 Tried a different approach to appease @davidfowl (and my own conscience)

BrennanConroy · 2017-11-28T17:23:38Z

Spec update in this PR?

analogrelay · 2017-11-28T17:30:22Z

No spec update. The only thing needed in the spec is the Ping frame. I'd argue that keep-alive behaviors are policy/config, not spec. I can look at throwing a sentence or two in though.

BrennanConroy · 2017-11-28T17:37:13Z

It says "Either endpoint may send a Ping message at any time.", I don't see a good reason the client should be sending pings

analogrelay · 2017-11-28T17:43:13Z

There are reasons for an implementation to want that. I'd rather keep the spec open. Just because it says an endpoint "may" do something doesn't mean our implementation has to do it. Also, our server code supports receiving (and ignoring) client pings.

BrennanConroy · 2017-11-28T17:46:09Z

:'( easy to shoot yourself in the foot

analogrelay · 2017-11-28T17:48:34Z

How so? There's no mechanism in our client to send a Ping message, and the server is tolerant of receiving them.

BrennanConroy · 2017-11-28T17:50:19Z

If someone makes a client exactly like ours except it sends keep-alive, then that will cause the server to stop sending keep-alives and then the client could timeout since it doesn't receive a keep-alive. Not a big problem I guess

analogrelay · 2017-11-28T17:52:47Z

Discussed with @BrennanConroy offline, he was slightly confused and we're all good 😃

analogrelay · 2017-11-28T17:54:06Z

To clarify the behavior to avoid confusion: The keep-alive timer is only reset when a message is sent by the server. Receiving messages from the client does not reset the timer.

BrennanConroy · 2017-11-28T17:53:39Z

specs/HubProtocol.md


 Ping messages do not have any payload, they are completely empty messages (aside from the encoding necessary to identify the message as a `Ping` message).

+It is up to the server implementation how frequently (if at all) `Ping` frames are sent. The ASP.NET Core implementation sends `Ping` frames only when using the Server Sent Events and WebSockets transports, at a default interval of 15 seconds (configurable). However, a `Ping` frame is only sent if 15 seconds elapses since the last message was sent. Clients may choose to use the "Ping rate" to provide a timeout for the server connection. Since the Client can expect the server to send `Ping` frames at a regular intervals, even when the connection is idle, it can use that to determine if the server has left without closing the connection. The ASP.NET Core implementation (both JavaScript and C#) use a default timeout window of 30 seconds, which is twice the server ping rate interval.


"implementation to decide how frequently"

"at a regular intervals"

BrennanConroy · 2017-11-28T19:14:32Z

src/Microsoft.AspNetCore.SignalR.Core/HubEndPoint.cs

                    {
-                        while (output.Reader.TryRead(out var hubMessage))
+                        var keepAliveTask = needKeepAlive ?
+                            Task.Delay(_hubOptions.KeepAliveInterval) :


Should I be concerned that we new up a Task.Delay every send?

Not every send. Every time we need to go async to wait for the app to send. And you have to allocate a task for this no matter how you slice it.

BrennanConroy · 2017-11-28T19:33:36Z

src/Microsoft.AspNetCore.Sockets.Abstractions/Features/ConnectionInherentKeepAliveFeature.cs

@@ -0,0 +1,14 @@
+using System;


BrennanConroy · 2017-11-28T19:33:41Z

src/Microsoft.AspNetCore.Sockets.Abstractions/Features/IConnectionInherentKeepAliveFeature.cs

@@ -0,0 +1,21 @@
+using System;


BrennanConroy · 2017-11-28T19:39:22Z

src/Microsoft.AspNetCore.Sockets.Http/Internal/Transports/LongPollingTransport.cs

-        public async Task ProcessRequestAsync(HttpContext context, CancellationToken token)
+        public async Task ProcessRequestAsync(ConnectionContext connection, HttpContext context, CancellationToken token)
        {
+            if(connection.Features.Get<IConnectionInherentKeepAliveFeature>() == null)


BrennanConroy · 2017-11-28T19:39:53Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+
+                // Echo a bunch of stuff, waiting 10ms between each, until 500ms have elapsed
+                DateTime start = DateTime.UtcNow;
+                while((DateTime.UtcNow - start).TotalMilliseconds <= 500.0)


BrennanConroy · 2017-11-28T19:40:11Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+                // We shouldn't have any ping messages
+                HubMessage message;
+                var counter = 0;
+                while((message = await client.ReadAsync()) != null)


BrennanConroy · 2017-11-28T19:41:45Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+                    counter += 1;
+                    Assert.IsNotType<PingMessage>(message);
+                }
+                Assert.InRange(counter, 1, Int32.MaxValue);


Maybe 50 would be better than max value

BrennanConroy · 2017-11-28T19:43:37Z

test/Microsoft.AspNetCore.Sockets.Tests/LongPollingTests.cs

 using Microsoft.AspNetCore.Sockets.Internal.Transports;
 using Microsoft.Extensions.Logging;
 using Xunit;
+using System;


analogrelay · 2017-11-28T22:06:05Z

🆙 📅

BrennanConroy · 2017-11-28T22:09:25Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+                    counter += 1;
+                    Assert.Same(PingMessage.Instance, message);
+                }
+                Assert.InRange(counter, 1, Int32.MaxValue);


I'm going to do 1-10, because I want to make sure this doesn't flake out occasionally due to random delays here and there. Remember that Task.Delay only guarantees that you won't be woken up before the delay has elapsed; you might be sleeping for longer than the delay.

BrennanConroy · 2017-11-28T22:13:29Z

src/Microsoft.AspNetCore.Sockets.Abstractions/Features/IConnectionInherentKeepAliveFeature.cs

+    /// </remarks>
+    public interface IConnectionInherentKeepAliveFeature
+    {
+        TimeSpan KeepAliveInterval { get; }


Does this property provide any value? It isn't used currently

It exposes the duration of the inherent keep-alive (i.e. for Long Polling, the poll timeout value). To be honest, this is just used as a marker interface though, so we could drop it. It seemed like a useful value to have though.

It would cleanup the LongPolling constructor a little :P

But marker interfaces are groooooooss

I'm with @BrennanConroy, I don't think this adds any value. Maybe this interface should have a bool so that keep alives can be manually turned off.

analogrelay · 2017-11-28T22:51:52Z

🆙 📅

BrennanConroy

I was about to complain about AppVeyor but you saw it too :D

BrennanConroy · 2017-11-28T23:56:38Z

test/Microsoft.AspNetCore.Sockets.Tests/LongPollingTests.cs

            using (var cts = CancellationTokenSource.CreateLinkedTokenSource(timeoutToken, context.RequestAborted))
            {
-                await poll.ProcessRequestAsync(context, cts.Token).OrTimeout();
+                await poll.ProcessRequestAsync(connection, context, cts.Token).OrTimeout().OrTimeout();


remove extra OrTimeout

What if the timeout times out?

davidfowl

I want to discuss this in person before you merge

analogrelay · 2017-11-30T00:35:57Z

🆙 📅 @davidfowl - This covers some of our in-person discussions. It needs a little clean-up, but the gist is what we talked about.

davidfowl · 2017-11-30T05:42:21Z

src/Microsoft.AspNetCore.SignalR.Core/HubEndPoint.cs

+                    // If it is, we send a ping frame, if not, we no-op on this tick. This means that in the worst case, the
+                    // true "ping rate" of the server could be (_hubOptions.KeepAliveInterval + HubEndPoint.KeepAliveTimerInterval),
+                    // because if the interval elapses right after the last tick of this timer, it won't be detected until the next tick.
+                    void KeepAliveTick(object state)


ugh, local functions. We should avoid capturing state.

davidfowl · 2017-11-30T05:45:45Z

src/Microsoft.AspNetCore.SignalR.Core/HubEndPoint.cs

+            _lastSendTimestamp = Stopwatch.GetTimestamp();
            var writingOutputTask = WriteToTransport();

+            Timer keepAliveTimer = null;


We're going to have wayyyyy too many timers. We need a connection heartbeat design here. The only thing that has a connection list is the lifetime manager though. We need to think a bit more here.

I thought we were going to look at a single timer model.

So did I until I realized how HubEndpoint works when reviewing it this morning :). We need to rethink this quite a bit. I think we need to go back to the slightly gross idea of having the connection manager tick the heartbeat via a feature. The problem there is the config gets gross. The ping rate is always going to be off by at most the tick rate and that gets weird.

analogrelay · 2017-11-30T18:24:09Z

🆙 📅 after discussions with @davidfowl, tests don't pass yet, but the pings are indeed sent.

analogrelay · 2017-12-01T00:02:19Z

Pushed some test fixes, but I'm not happy with it. I think the HubLifetimeManager tests are too low-level and brittle. I want to rewrite them to use HubEndpoint but I don't have the time to do that today, I'll look at it tomorrow.

analogrelay · 2017-12-01T05:25:30Z

I gave up trying to make the tests less brittle. It meant refactoring a huge amount of unit tests (because I needed to make TestClient changes to make it clean). I'm sure I could spend more energy and figure out an easier refactor, but I just don't have that kind of time :).

🆙 📅 ready for review again. Tests pass, and we're now using a single heartbeat tick for all connections.

Changes happened need to review

- This change moves more onto the HubConnectionContext and cleans up the model a bit. The HubConnectionContext is responsible for reading from the transport and exposing a Channel<HubMessage> for both input and output. - This cleans up the HubEndPoint a bit as it's solely responsible for invoking hub messages instead of parsing the bytes off the wire.

@davidfowl

@davidfowl said to just merge it :)

analogrelay requested review from BrennanConroy and davidfowl November 27, 2017 22:02

BrennanConroy reviewed Nov 27, 2017

View reviewed changes

davidfowl reviewed Nov 28, 2017

View reviewed changes

analogrelay mentioned this pull request Nov 28, 2017

Implement #1162 by adding client timeout for JavaScript #1163

Merged

BrennanConroy reviewed Nov 28, 2017

View reviewed changes

BrennanConroy previously approved these changes Nov 29, 2017

View reviewed changes

analogrelay mentioned this pull request Nov 29, 2017

Implement #1157 by adding client timeout for C# client #1165

Merged

davidfowl previously requested changes Nov 29, 2017

View reviewed changes

davidfowl reviewed Nov 30, 2017

View reviewed changes

analogrelay and others added 24 commits December 4, 2017 10:39

remove no-longer-needed KeepAliveChannel

3f2f83c

spec update

a46ed6f

pr feedback

fcf4e4a

clean up options

7c86865

pr feedback

3a8ba54

pr feedback

07650ec

trying something to make AppVeyor more reliable

971e24e

untimeout the timeout timeout from timing out the timeout

1d0c13d

use a single background timer per endpoint

83943e9

Move state to the HubConnectionContext

daf8c18

reworked after @davidfowl suggestions

7648a88

more clean-up; tests still broken

a001bf3

fix test build

21f57d1

stash test work

499091c

fix unit tests

6b08311

integrate @davidfowl refactorings

8bf06ec

reposition IConnectionInherentKeepAliveFeature init

942aa7e

commit missed file

c30064f

formatting

c675be5

pr feedback

5ee0774

fix build

7e2012e

pr feedback

68ac76c

derp

84f8279

analogrelay force-pushed the anurse/1156-server-ping branch from 162c677 to 84f8279 Compare December 4, 2017 18:39

analogrelay added 2 commits December 4, 2017 11:49

herpaderp

60556d7

fix builds

477c4ce

analogrelay merged commit c1d2024 into dev Dec 4, 2017

analogrelay deleted the anurse/1156-server-ping branch December 13, 2017 00:39


		Ping messages do not have any payload, they are completely empty messages (aside from the encoding necessary to identify the message as a `Ping` message).

		It is up to the server implementation how frequently (if at all) `Ping` frames are sent. The ASP.NET Core implementation sends `Ping` frames only when using the Server Sent Events and WebSockets transports, at a default interval of 15 seconds (configurable). However, a `Ping` frame is only sent if 15 seconds elapses since the last message was sent. Clients may choose to use the "Ping rate" to provide a timeout for the server connection. Since the Client can expect the server to send `Ping` frames at a regular intervals, even when the connection is idle, it can use that to determine if the server has left without closing the connection. The ASP.NET Core implementation (both JavaScript and C#) use a default timeout window of 30 seconds, which is twice the server ping rate interval.

Implement #1156 by having the server send Ping messages #1161

Implement #1156 by having the server send Ping messages #1161

Uh oh!

Conversation

analogrelay commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Nov 28, 2017

Uh oh!

BrennanConroy commented Nov 28, 2017

Uh oh!

analogrelay commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrennanConroy commented Nov 28, 2017

Uh oh!

analogrelay commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrennanConroy commented Nov 28, 2017

Uh oh!

analogrelay commented Nov 28, 2017

Uh oh!

BrennanConroy commented Nov 28, 2017

Uh oh!

analogrelay commented Nov 28, 2017

Uh oh!

analogrelay commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

analogrelay commented Nov 27, 2017 •

edited

Loading

analogrelay commented Nov 28, 2017 •

edited

Loading

analogrelay commented Nov 28, 2017 •

edited

Loading

analogrelay Nov 28, 2017 •

edited

Loading

davidfowl Nov 30, 2017 •

edited

Loading