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

analogrelay · 2017-11-29T00:03:25Z

Add client timeout to C# client. It is configurable via an option and defaults to 30 seconds (the server ping rate defaults to 15 seconds, so this is effectively 2 server ping intervals).

NOTE: Should not be merged until #1161 is merged since it will immediately break any connection that goes idle for 30 seconds :).

Fixes #1157

BrennanConroy · 2017-12-07T16:24:33Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs


        public Task Closed { get; }

+        /// <summary>
+        /// Gets or sets the server timeout interval for the connection. Changes to this value


"Changes to this value are ignored after StartAsync is called"
False, ResetTimeoutTimer is called in the OnDataReceivedAsync callback

BrennanConroy · 2017-11-29T00:11:02Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs

+                _logger.AbortingClient(ex, _connectionId);
+
+                // Immediately fault the close task. When the transport shuts down,
+                // it will trigger the close task to be completed, so we want to it be


"we want to it be" more gramrar

BrennanConroy · 2017-11-29T00:11:35Z

src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/SocketClientLoggerExtensions.cs

@@ -506,6 +509,14 @@ public static void SendingMessage(this ILogger logger, string connectionId)
            }
        }

+        public static void AbortingClient(this ILogger logger, Exception ex, string connectionId)
+        {
+            if (logger.IsEnabled(LogLevel.Information))


Error error ⚠️

BrennanConroy · 2017-11-29T00:12:16Z

src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/SocketClientLoggerExtensions.cs

@@ -506,6 +509,14 @@ public static void SendingMessage(this ILogger logger, string connectionId)
            }
        }

+        public static void AbortingClient(this ILogger logger, Exception ex, string connectionId)


I prefer Exception and connectionId swtiched

BrennanConroy · 2017-11-29T00:12:25Z

src/Microsoft.AspNetCore.Sockets.Client.Http/LongPollingTransport.cs

@@ -12,6 +12,7 @@
 using Microsoft.AspNetCore.Sockets.Client.Internal;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.AspNetCore.Sockets.Features;


davidfowl · 2017-11-29T06:22:43Z

src/Microsoft.AspNetCore.Sockets.Abstractions/IConnection.cs

@@ -13,6 +13,7 @@ public interface IConnection
        Task StartAsync();
        Task SendAsync(byte[] data, CancellationToken cancellationToken);
        Task DisposeAsync();
+        Task AbortAsync(Exception ex);


This is a sign we should unify the client and sever connection objects. This will turn into ConnectionContext

Agreed, but I don't want to block this PR on that right now since I don't need to unify them to implement this.

#1177 to track that

davidfowl · 2017-11-29T06:23:27Z

src/Microsoft.AspNetCore.Sockets.Client.Http/ITransport.cs

@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Sockets.Client
 {
    public interface ITransport
    {
-        Task StartAsync(Uri url, Channel<byte[], SendMessage> application, TransferMode requestedTransferMode, string connectionId);
+        Task StartAsync(Uri url, Channel<byte[], SendMessage> application, TransferMode requestedTransferMode, string connectionId, IConnection connection);


This isn't very clean. We shouldn't need to pass the IConnection here.

How else should transports add features? I actually think it makes sense, it's much like how it works on the server.

BrennanConroy · 2017-11-29T05:25:49Z

test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/HubConnectionTests.cs

+                    {
+                        await hubConnection.DisposeAsync().OrTimeout();
+                    }
+                    catch (TimeoutException) when (transportType != TransportType.LongPolling)


Where is this exception coming from? Doesn't seem right

DisposeAsync also throws the exception because it re-awaits Closed. I can move the assert to that though.

BrennanConroy · 2017-11-29T05:28:08Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

+
+            try
+            {
+                await connection.ReceiveJsonMessage(new { type = 6 });


Is there an enum for this value anywhere?

No, but there are constants... It's internal but I suppose we could make them pubternal.

BrennanConroy · 2017-11-29T05:31:00Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/LongPollingTransportTests.cs

@@ -16,6 +16,7 @@
 using Moq;
 using Moq.Protected;
 using Xunit;
+using Microsoft.AspNetCore.SignalR.Client.Tests;


BrennanConroy · 2017-11-29T05:32:57Z

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

@@ -12,6 +12,7 @@
 using Microsoft.Extensions.Logging.Testing;
 using Xunit;
 using Xunit.Abstractions;
+using Moq;


muratg · 2017-12-04T18:10:31Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

@@ -78,6 +92,20 @@ public HubConnection(IConnection connection, IHubProtocol protocol, ILoggerFacto
            }
        }

+        private void TimeoutElapsed()
+        {
+            _connection.AbortAsync(new TimeoutException("Server timeout elapsed without receiving a message from the server."));


Would it be useful to call out what the timeout was in the exception message?

Most other systems don't. We could, I suppose.

A lot of those are simply converting error codes to strings though so they don't have the timeout value :). I should be able to add that in easily.

If this is not common elsewhere, let's not complicate this more then. :)

Meh, it's easy and done. It'd be more work to remove it now :P

BrennanConroy · 2017-12-07T16:24:33Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs


        public Task Closed { get; }

+        /// <summary>
+        /// Gets or sets the server timeout interval for the connection. Changes to this value


"Changes to this value are ignored after StartAsync is called"
False, ResetTimeoutTimer is called in the OnDataReceivedAsync callback

BrennanConroy · 2017-12-07T16:28:24Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

@@ -64,6 +74,9 @@ public HubConnection(IConnection connection, IHubProtocol protocol, ILoggerFacto
                Shutdown(task.Exception);
                return task;
            }).Unwrap();
+
+            // Create the timer for timeout, but disabled by default (we enable it when started).
+            _timeoutTimer = new Timer(state => ((HubConnection)state).TimeoutElapsed(), this, Timeout.Infinite, Timeout.Infinite);


Just be aware that we'll need to set the timer to disabled in StopAsync when Pawel's PR goes in

BrennanConroy · 2017-12-07T16:29:06Z

src/Microsoft.AspNetCore.SignalR.Client.Core/Internal/SignalRClientLoggerExtensions.cs

@@ -85,6 +85,9 @@ internal static class SignalRClientLoggerExtensions
        private static readonly Action<ILogger, string, string, string, int, Exception> _preparingStreamingInvocation =
            LoggerMessage.Define<string, string, string, int>(LogLevel.Trace, new EventId(24, nameof(PreparingStreamingInvocation)), "Preparing streaming invocation '{invocationId}' of '{target}', with return type '{returnType}' and {argumentCount} argument(s).");

+        private static readonly Action<ILogger, Exception> _resettingKeepAliveTimer =
+            LoggerMessage.Define(LogLevel.Trace, new EventId(25, nameof(ResettingKeepAliveTimer)), "Resetting keep-alive timer, received a message from the server");


period at the end

BrennanConroy · 2017-12-07T17:13:23Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

+
+            hubConnection.ServerTimeout = TimeSpan.FromMilliseconds(100);
+
+            await hubConnection.StartAsync();


BrennanConroy · 2017-12-07T17:13:32Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

+            try
+            {
+                await connection.ReceiveJsonMessage(new { type = HubProtocolConstants.PingMessageType });
+                await Task.Delay(50);


BrennanConroy · 2017-12-07T17:13:51Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

+
+            hubConnection.ServerTimeout = TimeSpan.FromMilliseconds(100);
+
+            await hubConnection.StartAsync();


analogrelay requested review from davidfowl and BrennanConroy November 29, 2017 00:03

BrennanConroy reviewed Nov 29, 2017

View reviewed changes

davidfowl reviewed Nov 29, 2017

View reviewed changes

BrennanConroy reviewed Nov 29, 2017

View reviewed changes

muratg reviewed Dec 4, 2017

View reviewed changes

analogrelay force-pushed the anurse/1157-cs-client-timeout branch from d94a33f to 7808a40 Compare December 4, 2017 18:40

analogrelay and others added 6 commits December 4, 2017 16:20

stash before rebase

9da982b

initial pass at C# client timeout

6436e0a

fix tests

975941f

add some logging maybe?

c4f910b

feedback and fix build error

a54ca5d

add timeout value to timeoutexception

db3b58e

analogrelay force-pushed the anurse/1157-cs-client-timeout branch from 7808a40 to db3b58e Compare December 5, 2017 00:23

analogrelay added 3 commits December 5, 2017 09:35

fix unit tests

de0b3aa

adjust keepalive check to after HttpConnection has set the feature

fc3c7b9

disable unreliable test

e0933c9

BrennanConroy reviewed Dec 7, 2017

View reviewed changes

analogrelay added 4 commits December 7, 2017 09:42

additional feedback

dffc24e

remove flaky timeout tests

fed0b1b

more feedback

f0ffc9b

fix comment

85dc2d0

BrennanConroy approved these changes Dec 7, 2017

View reviewed changes

analogrelay merged commit a833006 into dev Dec 7, 2017

analogrelay deleted the anurse/1157-cs-client-timeout branch December 7, 2017 22:54

analogrelay mentioned this pull request Dec 7, 2017

Making HttpConnection restartable (C#) #1147

Merged


		hubConnection.ServerTimeout = TimeSpan.FromMilliseconds(100);

		await hubConnection.StartAsync();

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

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

Uh oh!

Conversation

analogrelay commented Nov 29, 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!

analogrelay Nov 29, 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!

analogrelay Dec 4, 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!

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!

Uh oh!

analogrelay commented Nov 29, 2017 •

edited

Loading

analogrelay Nov 29, 2017 •

edited

Loading

analogrelay Dec 4, 2017 •

edited

Loading