Dygray/handshake versioning (#2520)

* set minor versions on the protocols

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/DefaultHubDispatcherBenchmark.cs

@@ -54,6 +54,7 @@ namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
         {
             public string Name { get; }
             public int Version => 1;
+            public int MinorVersion => 0;
             public TransferFormat TransferFormat { get; }
 
             public bool IsVersionSupported(int version)

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/RedisHubLifetimeManagerBenchmark.cs

@@ -177,6 +177,7 @@ namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
             public string Name => _name;
 
             public int Version => _innerProtocol.Version;
+            public int MinorVersion => _innerProtocol.MinorVersion;
 
             public TransferFormat TransferFormat => _innerProtocol.TransferFormat;
 

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/RedisProtocolBenchmark.cs

@@ -126,6 +126,7 @@ namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
             public string Name { get; }
 
             public int Version => 1;
+            public int MinorVersion => 0;
 
             public TransferFormat TransferFormat => TransferFormat.Text;
 

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

@@ -52,6 +52,7 @@ namespace Microsoft.AspNetCore.SignalR.Client
 
         // Transient state to a connection
         private ConnectionState _connectionState;
+        private int _serverProtocolMinorVersion;
 
         public event Func<Exception, Task> Closed;
 
@@ -721,6 +722,8 @@ namespace Microsoft.AspNetCore.SignalR.Client
                                             $"Unable to complete handshake with the server due to an error: {message.Error}");
                                     }
 
+                                    _serverProtocolMinorVersion = message.MinorVersion;
+
                                     break;
                                 }
                             }
@@ -739,11 +742,12 @@ namespace Microsoft.AspNetCore.SignalR.Client
                     }
                 }
             }
+            
+            // shutdown if we're unable to read handshake
             // Ignore HubException because we throw it when we receive a handshake response with an error
-            // And we don't need to log that the handshake failed
+            // And because we already have the error, we don't need to log that the handshake failed
             catch (Exception ex) when (!(ex is HubException))
             {
-                // shutdown if we're unable to read handshake
                 Log.ErrorReceivingHandshakeResponse(_logger, ex);
                 throw;
             }
@@ -816,7 +820,7 @@ namespace Microsoft.AspNetCore.SignalR.Client
                     finally
                     {
                         // The buffer was sliced up to where it was consumed, so we can just advance to the start.
-                        // We mark examined as buffer.End so that if we didn't receive a full frame, we'll wait for more data
+                        // We mark examined as `buffer.End` so that if we didn't receive a full frame, we'll wait for more data
                         // before yielding the read again.
                         connectionState.Connection.Transport.Input.AdvanceTo(buffer.Start, buffer.End);
                     }
@@ -865,7 +869,6 @@ namespace Microsoft.AspNetCore.SignalR.Client
             // There is no need to start a new task if there is no Closed event registered
             if (closed != null)
             {
-
                 // Fire-and-forget the closed event
                 _ = RunClosedEvent(closed, connectionState.CloseException);
             }

src/Microsoft.AspNetCore.SignalR.Common/Protocol/HandshakeProtocol.cs

@@ -3,7 +3,9 @@
 
 using System;
 using System.Buffers;
+using System.Collections.Concurrent;
 using System.IO;
+using System.Text;
 using Microsoft.AspNetCore.Internal;
 using Microsoft.AspNetCore.SignalR.Internal;
 using Newtonsoft.Json;
@@ -17,26 +19,31 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
     {
         private const string ProtocolPropertyName = "protocol";
         private const string ProtocolVersionPropertyName = "version";
+        private const string MinorVersionPropertyName = "minorVersion";
         private const string ErrorPropertyName = "error";
         private const string TypePropertyName = "type";
 
-        /// <summary>
-        /// The serialized representation of a success handshake.
-        /// </summary>
-        public static ReadOnlyMemory<byte> SuccessHandshakeData;
+        private static ConcurrentDictionary<IHubProtocol, ReadOnlyMemory<byte>> _messageCache = new ConcurrentDictionary<IHubProtocol, ReadOnlyMemory<byte>>();
 
-        static HandshakeProtocol()
+        public static ReadOnlySpan<byte> GetSuccessfulHandshake(IHubProtocol protocol)
         {
-            var memoryBufferWriter = MemoryBufferWriter.Get();
-            try
+            ReadOnlyMemory<byte> result;
+            if(!_messageCache.TryGetValue(protocol, out result))
             {
-                WriteResponseMessage(HandshakeResponseMessage.Empty, memoryBufferWriter);
-                SuccessHandshakeData = memoryBufferWriter.ToArray();
-            }
-            finally
-            {
-                MemoryBufferWriter.Return(memoryBufferWriter);
+                var memoryBufferWriter = MemoryBufferWriter.Get();
+                try
+                {
+                    WriteResponseMessage(new HandshakeResponseMessage(protocol.MinorVersion), memoryBufferWriter);
+                    result = memoryBufferWriter.ToArray();
+                    _messageCache.TryAdd(protocol, result);
+                }
+                finally
+                {
+                    MemoryBufferWriter.Return(memoryBufferWriter);
+                }
             }
+
+            return result.Span;
         }
 
         /// <summary>
@@ -87,6 +94,9 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
                         writer.WriteValue(responseMessage.Error);
                     }
 
+                    writer.WritePropertyName(MinorVersionPropertyName);
+                    writer.WriteValue(responseMessage.MinorVersion);
+
                     writer.WriteEndObject();
                     writer.Flush();
                 }
@@ -122,6 +132,7 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
                     JsonUtils.CheckRead(reader);
                     JsonUtils.EnsureObjectStart(reader);
 
+                    int? minorVersion = null;
                     string error = null;
 
                     var completed = false;
@@ -141,6 +152,9 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
                                     case ErrorPropertyName:
                                         error = JsonUtils.ReadAsString(reader, ErrorPropertyName);
                                         break;
+                                    case MinorVersionPropertyName:
+                                        minorVersion = JsonUtils.ReadAsInt32(reader, MinorVersionPropertyName);
+                                        break;
                                     default:
                                         reader.Skip();
                                         break;
@@ -154,7 +168,7 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
                         }
                     };
 
-                    responseMessage = (error != null) ? new HandshakeResponseMessage(error) : HandshakeResponseMessage.Empty;
+                    responseMessage = new HandshakeResponseMessage(minorVersion, error);
                     return true;
                 }
             }

src/Microsoft.AspNetCore.SignalR.Common/Protocol/HandshakeResponseMessage.cs

@@ -11,20 +11,41 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
         /// <summary>
         /// An empty response message with no error.
         /// </summary>
-        public static readonly HandshakeResponseMessage Empty = new HandshakeResponseMessage(null);
+        public static readonly HandshakeResponseMessage Empty = new HandshakeResponseMessage(error: null);
 
         /// <summary>
         /// Gets the optional error message.
         /// </summary>
         public string Error { get; }
 
+        /// <summary>
+        /// Highest minor protocol version that the server supports.
+        /// </summary>
+        public int MinorVersion { get; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="HandshakeResponseMessage"/> class.
+        /// An error response does need a minor version. Since the handshake has failed, any extra data will be ignored.
+        /// </summary>
+        /// <param name="error">Error encountered by the server, indicating why the handshake has failed.</param>
+        public HandshakeResponseMessage(string error) : this(null, error) { }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="HandshakeResponseMessage"/> class.
+        /// A reponse with a minor version indicates success, and doesn't require an error field.
+        /// </summary>
+        /// <param name="minorVersion">The highest protocol minor version that the server supports.</param>
+        public HandshakeResponseMessage(int minorVersion) : this(minorVersion, null) { }
+
         /// <summary>
         /// Initializes a new instance of the <see cref="HandshakeResponseMessage"/> class.
         /// </summary>
-        /// <param name="error">An optional response error message. A <c>null</c> error message indicates a succesful handshake.</param>
-        public HandshakeResponseMessage(string error)
+        /// <param name="error">Error encountered by the server, indicating why the handshake has failed.</param>
+        /// <param name="minorVersion">The highest protocol minor version that the server supports.</param>
+        public HandshakeResponseMessage(int? minorVersion, string error)
         {
-            // Note that a response with an empty string for error in the JSON is considered an errored response
+            // MinorVersion defaults to 0, because old servers don't send a minor version 
+            MinorVersion = minorVersion ?? 0;
             Error = error;
         }
     }

src/Microsoft.AspNetCore.SignalR.Common/Protocol/IHubProtocol.cs

@@ -18,10 +18,15 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
         string Name { get; }
 
         /// <summary>
-        /// Gets the version of the protocol.
+        /// Gets the major version of the protocol.
         /// </summary>
         int Version { get; }
 
+        /// <summary>
+        /// Gets the minor version of the protocol.
+        /// </summary>
+        int MinorVersion { get; }
+
         /// <summary>
         /// Gets the transfer format of the protocol.
         /// </summary>

src/Microsoft.AspNetCore.SignalR.Common/breakingchanges.netcore.json

@@ -0,0 +1,12 @@
+[
+  {
+    "TypeId": "public static class Microsoft.AspNetCore.SignalR.Protocol.HandshakeProtocol",
+    "MemberId": "public static System.ReadOnlyMemory<System.Byte> SuccessHandshakeData",
+    "Kind": "Removal"
+  },
+  {
+    "TypeId": "public interface Microsoft.AspNetCore.SignalR.Protocol.IHubProtocol",
+    "MemberId": "System.Int32 get_MinorVersion()",
+    "Kind": "Addition"
+  }
+]

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

@@ -282,10 +282,9 @@ namespace Microsoft.AspNetCore.SignalR
 
             try
             {
-                if (message == HandshakeResponseMessage.Empty)
+                if (message.Error == null)
                 {
-                    // success response is always an empty object so send cached data
-                    _connectionContext.Transport.Output.Write(HandshakeProtocol.SuccessHandshakeData.Span);
+                    _connectionContext.Transport.Output.Write(HandshakeProtocol.GetSuccessfulHandshake(Protocol));
                 }
                 else
                 {
@@ -398,7 +397,8 @@ namespace Microsoft.AspNetCore.SignalR
                                     }
 
                                     Log.HandshakeComplete(_logger, Protocol.Name);
-                                    await WriteHandshakeResponseAsync(HandshakeResponseMessage.Empty);
+
+                                    await WriteHandshakeResponseAsync(new HandshakeResponseMessage(Protocol.MinorVersion));
                                     return true;
                                 }
                             }

src/Microsoft.AspNetCore.SignalR.Protocols.Json/Protocol/JsonHubProtocol.cs

@@ -32,6 +32,7 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
 
         private static readonly string ProtocolName = "json";
         private static readonly int ProtocolVersion = 1;
+        private static readonly int ProtocolMinorVersion = 0;
 
         /// <summary>
         /// Gets the serializer used to serialize invocation arguments and return values.
@@ -60,6 +61,9 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
         /// <inheritdoc />
         public int Version => ProtocolVersion;
 
+        /// <inheritdoc />        
+        public int MinorVersion => ProtocolMinorVersion;
+
         /// <inheritdoc />
         public TransferFormat TransferFormat => TransferFormat.Text;
 

src/Microsoft.AspNetCore.SignalR.Protocols.MessagePack/Protocol/MessagePackHubProtocol.cs

@@ -29,13 +29,17 @@ namespace Microsoft.AspNetCore.SignalR.Protocol
 
         private static readonly string ProtocolName = "messagepack";
         private static readonly int ProtocolVersion = 1;
-
+        private static readonly int ProtocolMinorVersion = 0;
+        
         /// <inheritdoc />
         public string Name => ProtocolName;
 
         /// <inheritdoc />
         public int Version => ProtocolVersion;
 
+        /// <inheritdoc />
+        public int MinorVersion => ProtocolMinorVersion;
+
         /// <inheritdoc />
         public TransferFormat TransferFormat => TransferFormat.Binary;
 

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

@@ -25,6 +25,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests
         public string Name => _innerProtocol.Name;
         public int Version => _version;
         public TransferFormat TransferFormat => _innerProtocol.TransferFormat;
+        public int MinorVersion => 0; // not used in this test class, just for interface conformance
 
         public bool TryParseMessage(ref ReadOnlySequence<byte> input, IInvocationBinder binder, out HubMessage message)
         {

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

@@ -63,6 +63,28 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
                 }
             }
 
+            [Fact]
+            public async Task ClientIsOkayReceivingMinorVersionInHandshake()
+            {
+                // We're just testing that the client doesn't fail when a minor version is added to the handshake 
+                // The client doesn't actually use that version anywhere yet so there's nothing else to test at this time
+
+                var connection = new TestConnection(autoHandshake: false);
+                var hubConnection = CreateHubConnection(connection);
+                try
+                {
+                    var startTask = hubConnection.StartAsync();
+                    var message = await connection.ReadHandshakeAndSendResponseAsync(56);
+
+                    await startTask;
+                }
+                finally
+                {
+                    await hubConnection.DisposeAsync().OrTimeout();
+                    await connection.DisposeAsync().OrTimeout();
+                }
+            }
+
             [Fact]
             public async Task InvokeSendsAnInvocationMessage()
             {

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

@@ -169,6 +169,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
 
             public string Name => "MockHubProtocol";
             public int Version => 1;
+            public int MinorVersion => 1;
 
             public TransferFormat TransferFormat => TransferFormat.Binary;
 

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

@@ -78,7 +78,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             return this;
         }
 
-        public async Task<string> ReadHandshakeAndSendResponseAsync()
+        public async Task<string> ReadHandshakeAndSendResponseAsync(int minorVersion = 0)
         {
             var s = await ReadSentTextMessageAsync();
 
@@ -87,7 +87,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             var output = MemoryBufferWriter.Get();
             try
             {
-                HandshakeProtocol.WriteResponseMessage(HandshakeResponseMessage.Empty, output);
+                HandshakeProtocol.WriteResponseMessage(new HandshakeResponseMessage(minorVersion), output);
                 response = output.ToArray();
             }
             finally

test/Microsoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/HandshakeProtocolTests.cs

@@ -38,6 +38,17 @@ namespace Microsoft.AspNetCore.SignalR.Common.Tests.Internal.Protocol
             Assert.Equal(error, response.Error);
         }
 
+        [Theory]
+        [InlineData("{\"error\":\"\",\"minorVersion\":34}\u001e", 34)]
+        [InlineData("{\"error\":\"flump flump flump\",\"minorVersion\":112}\u001e", 112)]
+        public void ParsingResponseMessageGivesMinorVersion(string json, int version)
+        {
+            var message = new ReadOnlySequence<byte>(Encoding.UTF8.GetBytes(json));
+
+            Assert.True(HandshakeProtocol.TryParseResponseMessage(ref message, out var response));
+            Assert.Equal(version, response.MinorVersion);
+        }
+
         [Fact]
         public void ParsingHandshakeRequestNotCompleteReturnsFalse()
         {

test/Microsoft.AspNetCore.SignalR.Tests.Utils/DummyHubProtocol.cs

@@ -16,6 +16,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests
 
         public string Name { get; }
         public int Version => 1;
+        public int MinorVersion => 0;
         public TransferFormat TransferFormat => TransferFormat.Text;
 
         public DummyHubProtocol(string name, Action onWrite = null)

test/Microsoft.AspNetCore.SignalR.Tests.Utils/TestClient.cs

@@ -25,7 +25,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests
         private List<(Action<object> handler, object state)> _heartbeatHandlers;
 
         private static int _id;
-        private readonly IHubProtocol _protocol;
+        private IHubProtocol _protocol;
         private readonly IInvocationBinder _invocationBinder;
         private readonly CancellationTokenSource _cts;
 

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Utils.cs

@@ -64,9 +64,9 @@ namespace Microsoft.AspNetCore.SignalR.Tests
             return services.BuildServiceProvider();
         }
 
-        public static Connections.ConnectionHandler GetHubConnectionHandler(Type hubType)
+        public static Connections.ConnectionHandler GetHubConnectionHandler(Type hubType, Action<ServiceCollection> addServices = null)
         {
-            var serviceProvider = CreateServiceProvider();
+            var serviceProvider = CreateServiceProvider(addServices);
             return (Connections.ConnectionHandler)serviceProvider.GetService(GetConnectionHandlerType(hubType));
         }
     }

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

@@ -2332,6 +2332,30 @@ namespace Microsoft.AspNetCore.SignalR.Tests
             }
         }
 
+        [Fact]
+        public async Task ServerReportsProtocolMinorVersion()
+        {
+            var testProtocol = new Mock<IHubProtocol>();
+            testProtocol.Setup(m => m.Name).Returns("CustomProtocol");
+            testProtocol.Setup(m => m.MinorVersion).Returns(112);
+            testProtocol.Setup(m => m.IsVersionSupported(It.IsAny<int>())).Returns(true);
+            testProtocol.Setup(m => m.TransferFormat).Returns(TransferFormat.Binary);
+            
+            var connectionHandler = HubConnectionHandlerTestUtils.GetHubConnectionHandler(typeof(HubT),
+                (services) => services.AddSingleton<IHubProtocol>(testProtocol.Object));
+
+            using (var client = new TestClient(protocol: testProtocol.Object))
+            {
+                var connectionHandlerTask = await client.ConnectAsync(connectionHandler);
+
+                Assert.NotNull(client.HandshakeResponseMessage);
+                Assert.Equal(112, client.HandshakeResponseMessage.MinorVersion);
+
+                client.Dispose();
+                await connectionHandlerTask.OrTimeout();
+            }
+        }
+
         private class CustomHubActivator<THub> : IHubActivator<THub> where THub : Hub
         {
             public int ReleaseCount;