Made changes to the KeepAlive feature as per code review feedback

- Added a SuccessiveTimeoutTest to test the case when the connection times out successively
- Added constructors to the KeepAliveData class
#741

src/Microsoft.AspNet.SignalR.Client/Connection.cs

@@ -1,11 +1,5 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.md in the project root for license information.
 
-using Microsoft.AspNet.SignalR.Client.Http;
-using Microsoft.AspNet.SignalR.Client.Infrastructure;
-using Microsoft.AspNet.SignalR.Client.Transports;
-using Microsoft.AspNet.SignalR.Infrastructure;
-using Newtonsoft.Json;
-using Newtonsoft.Json.Linq;
 using System;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
@@ -14,6 +8,12 @@
 using System.Net;
 using System.Reflection;
 using System.Threading.Tasks;
+using Microsoft.AspNet.SignalR.Client.Http;
+using Microsoft.AspNet.SignalR.Client.Infrastructure;
+using Microsoft.AspNet.SignalR.Client.Transports;
+using Microsoft.AspNet.SignalR.Infrastructure;
+using Newtonsoft.Json;
+using Newtonsoft.Json.Linq;
 
 namespace Microsoft.AspNet.SignalR.Client
 {
@@ -39,18 +39,14 @@ public class Connection : IConnection
         // The default connection state is disconnected
         private ConnectionState _state;
 
+        private KeepAliveData _keepAliveData;
+
         // Used to synchronize state changes
         private readonly object _stateLock = new object();
 
-        // Determines when we warn the developer that the connection may be lost
-        private double _keepAliveWarnAt = 2.0 / 3.0;
-
         // Keeps track of when the last keep alive from the server was received
         private HeartbeatMonitor _monitor;
 
-        // Object to store the various keep alive timeout values
-        public KeepAliveData KeepAliveData { get; private set; }
-
         /// <summary>
         /// Occurs when the <see cref="Connection"/> has received data from the server.
         /// </summary>
@@ -84,7 +80,7 @@ public class Connection : IConnection
         /// <summary>
         /// Occurs when the <see cref="Connection"/> is about to timeout
         /// </summary>
-        public event Action TimeoutWarning;
+        public event Action ConnectionSlow;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="Connection"/> class.
@@ -135,6 +131,22 @@ public Connection(string url, string queryString)
             State = ConnectionState.Disconnected;
         }
 
+        /// <summary>
+        /// Object to store the various keep alive timeout values
+        /// </summary>
+        KeepAliveData IConnection.KeepAliveData
+        {
+            get
+            {
+                return _keepAliveData;
+            }
+
+            set
+            {
+                _keepAliveData = value;
+            }
+        }
+
         /// <summary>
         /// Gets or sets the cookies associated with the connection.
         /// </summary>
@@ -249,13 +261,13 @@ public Task Start(IHttpClient httpClient)
         public Task Start(IClientTransport transport)
         {
             _disconnectCts = new SafeCancellationTokenSource();
-            _monitor = new HeartbeatMonitor(this);
 
             if (!ChangeState(ConnectionState.Disconnected, ConnectionState.Connecting))
             {
                 return TaskAsyncHelper.Empty;
             }
 
+            _monitor = new HeartbeatMonitor(this);
             _transport = transport;
 
             return Negotiate(transport);
@@ -282,17 +294,8 @@ private Task Negotiate(IClientTransport transport)
                 // If we have a keep alive
                 if (negotiationResponse.KeepAliveTimeout != null)
                 {
-                    KeepAliveData = new KeepAliveData();
-
-                    // Timeout to designate when to force the connection into reconnecting
-                    KeepAliveData.Timeout = TimeSpan.FromSeconds(negotiationResponse.KeepAliveTimeout.Value);
-
-                    // Timeout to designate when to warn the developer that the connection may be dead or is hanging.
-                    KeepAliveData.TimeoutWarning = TimeSpan.FromMilliseconds(KeepAliveData.Timeout.TotalMilliseconds * _keepAliveWarnAt);
-
-                    // Instantiate the frequency in which we check the keep alive.  It must be short in order to not miss/pick up any changes
-                    KeepAliveData.CheckInterval = TimeSpan.FromMilliseconds((KeepAliveData.Timeout.TotalMilliseconds - KeepAliveData.TimeoutWarning.TotalMilliseconds) / 3);
-                }
+                    _keepAliveData = new KeepAliveData(TimeSpan.FromSeconds(negotiationResponse.KeepAliveTimeout.Value));
+               }
 
                 var data = OnSending();
                 StartTransport(data).ContinueWith(negotiateTcs);
@@ -497,13 +500,15 @@ void IConnection.OnReconnected()
             {
                 Reconnected();
             }
+
+            ((IConnection)this).UpdateLastKeepAlive();
         }
 
-        void IConnection.OnTimeoutWarning()
+        void IConnection.OnConnectionSlow()
         {
-            if (TimeoutWarning != null)
+            if (ConnectionSlow != null)
             {
-                TimeoutWarning();
+                ConnectionSlow();
             }
         }
 
@@ -512,7 +517,10 @@ void IConnection.OnTimeoutWarning()
         /// </summary>
         void IConnection.UpdateLastKeepAlive()
         {
-            KeepAliveData.LastKeepAlive = DateTime.UtcNow;
+            if (_keepAliveData != null)
+            {
+                _keepAliveData.LastKeepAlive = DateTime.UtcNow;
+            }
         }
 
         [SuppressMessage("Microsoft.Design", "CA1062:Validate arguments of public methods", MessageId = "0", Justification = "This is called by the transport layer")]

src/Microsoft.AspNet.SignalR.Client/HeartBeatMonitor.cs

@@ -12,17 +12,14 @@ public class HeartbeatMonitor : IDisposable
         private Timer _timer;
 #endif
 
-        // Keep track of whether we have already disposed
-        private bool _disposed;
-
         // Connection variable
         private readonly IConnection _connection;
 
         // To keep track of whether the user has been notified
         public bool HasBeenWarned { get; private set; }
 
         // To keep track of whether the client is already reconnecting
-        public bool Reconnecting { get; private set; }
+        public bool TimedOut { get; private set; }
 
         /// <summary>
         /// Initializes a new instance of the HeartBeatMonitor Class 
@@ -39,9 +36,8 @@ public HeartbeatMonitor(IConnection connection)
         public void Start()
         {
             _connection.UpdateLastKeepAlive();
-            _disposed = false;
             HasBeenWarned = false;
-            Reconnecting = false;
+            TimedOut = false;
 #if !NETFX_CORE
             _timer = new Timer(_ => Beat(), state: null, dueTime: _connection.KeepAliveData.CheckInterval, period: _connection.KeepAliveData.CheckInterval);
 #endif
@@ -69,28 +65,28 @@ public void Beat(TimeSpan timeElapsed)
             {
                 if (timeElapsed >= _connection.KeepAliveData.Timeout)
                 {
-                    if (!Reconnecting)
+                    if (!TimedOut)
                     {
                         // Connection has been lost
-                        Debug.WriteLine("Connection Timed-out : Reconnecting {0}", DateTime.UtcNow);
-                        Reconnecting = true;
+                        Debug.WriteLine("Connection Timed-out : Transport Lost Connection {0}", DateTime.UtcNow);
+                        TimedOut = true;
                         _connection.Transport.LostConnection(_connection);
                     }
                 }
                 else if (timeElapsed >= _connection.KeepAliveData.TimeoutWarning)
                 {
                     if (!HasBeenWarned)
                     {
-                        // Inform user and set UserNotified to true
+                        // Inform user and set HasBeenWarned to true
                         Debug.WriteLine("Connection Timeout Warning : Notifying user {0}", DateTime.UtcNow);
                         HasBeenWarned = true;
-                        _connection.OnTimeoutWarning();
+                        _connection.OnConnectionSlow();
                     }
                 }
                 else
                 {
                     HasBeenWarned = false;
-                    Reconnecting = false;
+                    TimedOut = false;
                 }
             }
         }
@@ -110,22 +106,20 @@ public void Dispose()
         /// <param name="disposing"></param>
         protected virtual void Dispose(bool disposing)
         {
-            if (!_disposed)
+            if (disposing)
             {
-                if (disposing)
-                {
 #if !NETFX_CORE
+                lock (_timer)
+                {
                     if (_timer != null)
                     {
                         _timer.Dispose();
                         _timer = null;
-                    }                    
-#endif
+                    }
                 }
-
-                // Indicate that the instance has been disposed
-                _disposed = true;
+#endif
             }
         }
     }
 }
+

src/Microsoft.AspNet.SignalR.Client/IConnection.cs

@@ -1,19 +1,19 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.md in the project root for license information.
 
-using Microsoft.AspNet.SignalR.Client.Http;
-using Microsoft.AspNet.SignalR.Client.Transports;
-using Newtonsoft.Json.Linq;
 using System;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Net;
 using System.Threading.Tasks;
+using Microsoft.AspNet.SignalR.Client.Http;
+using Microsoft.AspNet.SignalR.Client.Transports;
+using Newtonsoft.Json.Linq;
 
 namespace Microsoft.AspNet.SignalR.Client
 {
     public interface IConnection
     {
-        KeepAliveData KeepAliveData { get; }
+        KeepAliveData KeepAliveData { get; set; }
         string MessageId { get; set; }
         string GroupsToken { get; set; }
         IDictionary<string, object> Items { get; }
@@ -38,7 +38,7 @@ public interface IConnection
         void OnError(Exception ex);
         void OnReconnecting();
         void OnReconnected();
-        void OnTimeoutWarning();
+        void OnConnectionSlow();
         void PrepareRequest(IRequest request);
         void UpdateLastKeepAlive();
     }

src/Microsoft.AspNet.SignalR.Client/KeepAliveData.cs

@@ -7,9 +7,33 @@ namespace Microsoft.AspNet.SignalR.Client
     /// </summary>
     public class KeepAliveData
     {
+        // Determines when we warn the developer that the connection may be lost
+        private const double _keepAliveWarnAt = 2.0 / 3.0;
+
         public DateTime LastKeepAlive { get; set; }
-        public TimeSpan Timeout { get; set; }
-        public TimeSpan TimeoutWarning { get; set; }
-        public TimeSpan CheckInterval { get; set; }
+
+        // Timeout to designate when to force the connection into reconnecting
+        public TimeSpan Timeout { get; private set; }
+
+        // Timeout to designate when to warn the developer that the connection may be dead or is hanging.
+        public TimeSpan TimeoutWarning { get; private set; }
+
+        // Frequency with which we check the keep alive.  It must be short in order to not miss/pick up any changes
+        public TimeSpan CheckInterval { get; private set; }
+
+        public KeepAliveData(TimeSpan timeout)
+        {
+            Timeout = timeout;
+            TimeoutWarning = TimeSpan.FromTicks((long)(Timeout.Ticks * _keepAliveWarnAt));
+            CheckInterval = TimeSpan.FromTicks((Timeout.Ticks - TimeoutWarning.Ticks) / 3);
+        }
+
+        public KeepAliveData(DateTime lastKeepAlive, TimeSpan timeout, TimeSpan timeoutWarning, TimeSpan checkInterval)
+        {
+            LastKeepAlive = lastKeepAlive;
+            Timeout = timeout;
+            TimeoutWarning = timeoutWarning;
+            CheckInterval = checkInterval;
+        }
     }
 }

src/Microsoft.AspNet.SignalR.Client/Microsoft.AspNet.SignalR.Client.csproj

@@ -129,7 +129,6 @@
   <Import Project="..\Common\Microsoft.AspNet.SignalR.targets" />
   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
   <Import Project="$(SolutionDir).nuget\NuGet.targets" />
-  <Import Project="$(SolutionDir)\.nuget\nuget.targets" />
   <!-- To modify your build process, add your task inside one of the targets below and uncomment it. 
        Other similar extension points exist, see Microsoft.Common.targets.
   <Target Name="BeforeBuild">

src/Microsoft.AspNet.SignalR.Client/Transports/AutoTransport.cs

@@ -36,7 +36,13 @@ public AutoTransport(IHttpClient httpClient)
         /// <summary>
         /// Indicates whether or not the active transport supports keep alive
         /// </summary>
-        public bool SupportsKeepAlive { get; private set; }
+        public bool SupportsKeepAlive
+        {
+            get
+            {
+                return _transport != null ? _transport.SupportsKeepAlive : false;
+            }
+        }
 
         public string Name
         {
@@ -117,16 +123,6 @@ private void ResolveTransport(IConnection connection, string data, CancellationT
                     // Set the active transport
                     _transport = transport;
 
-                    // Sets SupportsKeepAlive based on the active transport
-                    if (_transport.Name.Equals("webSockets", StringComparison.OrdinalIgnoreCase) || _transport.Name.Equals("serverSentEvents", StringComparison.OrdinalIgnoreCase))
-                    {
-                        SupportsKeepAlive = true;
-                    }
-                    else
-                    {
-                        SupportsKeepAlive = false;
-                    }
-
                     // Complete the process
                     tcs.SetResult(null);
                 }

src/Microsoft.AspNet.SignalR.Client/Transports/ServerSentEventsTransport.cs

@@ -16,6 +16,7 @@ namespace Microsoft.AspNet.SignalR.Client.Transports
     public class ServerSentEventsTransport : HttpBasedTransport
     {
         private EventSourceStreamReader _eventSource;
+
         public ServerSentEventsTransport()
             : this(new DefaultHttpClient())
         {
@@ -139,7 +140,7 @@ private void Reconnect(IConnection connection, string data, CancellationToken di
 
                     _eventSource.Opened = () =>
                     {
-                        //if we are reconnecting - we do not need to worry about the callback being invoked
+                        // If we're not reconnecting, then we're starting the transport for the first time. Trigger callback only on first start.
                         if (!reconnecting)
                         {
                             callbackInvoker.Invoke(initializeCallback);

src/Microsoft.AspNet.SignalR.Client/Transports/TransportHelper.cs

@@ -100,12 +100,8 @@ public static void ProcessResponse(IConnection connection, string response, out
                 throw new ArgumentNullException("connection");
             }
 
-            // Update the LastKeepAlive Value
-            if (connection.KeepAliveData != null)
-            {
-                connection.UpdateLastKeepAlive();
-                Debug.WriteLine("Received Message from the Server : {0}", DateTime.UtcNow);
-            }
+            connection.UpdateLastKeepAlive();
+            Debug.WriteLine("Received Message from the Server : {0}", DateTime.UtcNow);
 
             timedOut = false;
             disconnected = false;
@@ -152,7 +148,7 @@ public static void ProcessResponse(IConnection connection, string response, out
                         catch (Exception ex)
                         {
 #if NET35
-                                Debug.WriteLine(String.Format(CultureInfo.InvariantCulture, "Failed to process message: {0}", ex));
+                            Debug.WriteLine(String.Format(CultureInfo.InvariantCulture, "Failed to process message: {0}", ex));
 #else
                             Debug.WriteLine("Failed to process message: {0}", ex);
 #endif
@@ -167,7 +163,7 @@ public static void ProcessResponse(IConnection connection, string response, out
             catch (Exception ex)
             {
 #if NET35
-                    Debug.WriteLine(String.Format(CultureInfo.InvariantCulture, "Failed to response: {0}", ex));
+                Debug.WriteLine(String.Format(CultureInfo.InvariantCulture, "Failed to response: {0}", ex));
 #else
                 Debug.WriteLine("Failed to response: {0}", ex);
 #endif