From 733a3b3c2d911dc0b9585de55fc0fa23af6ca3d0 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 25 Mar 2018 00:51:53 -0700 Subject: [PATCH 1/6] Upgrade the dependencies (#1712) --- build/dependencies.props | 96 ++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index ec004a975a..18f2d9e075 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -5,60 +5,60 @@ 0.10.13 3.1.0 - 2.1.0-preview2-15742 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-a-preview2-default-connection-context-17612 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 0.5.0-preview2-30355 - 2.1.0-a-preview2-default-connection-context-17612 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 + 2.1.0-preview2-15744 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 0.5.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 4.5.0-preview2-26313-01 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 - 2.1.0-preview2-30355 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 + 2.1.0-preview2-30451 2.0.0 2.1.0-preview2-26314-02 15.6.1 4.7.49 1.0.0-rc - 10.0.1 + 11.0.1 1.2.4 4.5.0-preview2-26313-01 4.5.0-preview2-26313-01 From 65204ec6f290403a2a70c79cbbba7d939c50c68e Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 25 Mar 2018 12:38:51 -0700 Subject: [PATCH 2/6] Small changes (#1714) - Don't allocate for empty arrays. - Don't allocate the list of pre-serialized messages until writing --- .../Internal/Protocol/HubMessage.cs | 11 +++++++--- .../Internal/Protocol/JsonHubProtocol.cs | 21 ++++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HubMessage.cs b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HubMessage.cs index 14ba3ea78d..c1ebe09cf8 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HubMessage.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HubMessage.cs @@ -12,9 +12,8 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol { } - // Initialize with capacity 2 for the 2 built in protocols private object _lock = new object(); - private readonly List _serializedMessages = new List(2); + private List _serializedMessages; public byte[] WriteMessage(IHubProtocol protocol) { @@ -25,7 +24,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol lock (_lock) { - for (var i = 0; i < _serializedMessages.Count; i++) + for (var i = 0; i < _serializedMessages?.Count; i++) { if (_serializedMessages[i].Protocol.Equals(protocol)) { @@ -35,6 +34,12 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol var bytes = protocol.WriteToArray(this); + if (_serializedMessages == null) + { + // Initialize with capacity 2 for the 2 built in protocols + _serializedMessages = new List(2); + } + // We don't want to balloon memory if someone writes a poor IHubProtocolResolver // So we cap how many caches we store and worst case just serialize the message for every connection if (_serializedMessages.Count < 10) diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs index df6a664f2f..ebd9f86d4d 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs @@ -559,7 +559,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol private object[] BindArguments(JsonTextReader reader, IReadOnlyList paramTypes) { - var arguments = new object[paramTypes.Count]; + object[] arguments = null; var paramIndex = 0; var argumentsCount = 0; @@ -572,7 +572,12 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol throw new InvalidDataException($"Invocation provides {argumentsCount} argument(s) but target expects {paramTypes.Count}."); } - return arguments; + return arguments ?? Array.Empty(); + } + + if (arguments == null) + { + arguments = new object[paramTypes.Count]; } try @@ -608,12 +613,18 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol private object[] BindArguments(JArray args, IReadOnlyList paramTypes) { - var arguments = new object[args.Count]; - if (paramTypes.Count != arguments.Length) + if (paramTypes.Count != args.Count) { - throw new InvalidDataException($"Invocation provides {arguments.Length} argument(s) but target expects {paramTypes.Count}."); + throw new InvalidDataException($"Invocation provides {args.Count} argument(s) but target expects {paramTypes.Count}."); } + if (paramTypes.Count == 0) + { + return Array.Empty(); + } + + var arguments = new object[args.Count]; + try { for (var i = 0; i < paramTypes.Count; i++) From ddc0e4fb3a156f00102549c8e37df441682dc0f8 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 25 Mar 2018 21:07:17 -0700 Subject: [PATCH 3/6] Run benchmarks on .NET Core 2.1 (#1722) - Spoiler alert, it's much faster for the ones I ran at least --- .../Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj b/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj index 69db719b11..646477a011 100644 --- a/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj +++ b/benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj @@ -2,7 +2,7 @@ Exe - netcoreapp2.0 + netcoreapp2.1 From b8285b8356de091476c5e016d45aa3a5eb954c28 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 26 Mar 2018 14:36:08 -0700 Subject: [PATCH 4/6] Don't create the span on netstandard (#1721) - Directly pin the char[] - Changed Utf8BufferTextReader to use the Utf8Decoder - It copies whatever it can into the char buffer allocated in a stateful way (it's more efficient). - Added tests for unicode and ascii reading - Added a thread static cache --- .../Internal/Protocol/HandshakeProtocol.cs | 3 +- .../Internal/Protocol/JsonHubProtocol.cs | 17 ++- .../Internal/Protocol/Utf8BufferTextReader.cs | 73 ++++++++--- .../Properties/AssemblyInfo.cs | 3 +- .../Protocol/Utf8BufferTextReaderTests.cs | 119 ++++++++++++++++++ 5 files changed, 190 insertions(+), 25 deletions(-) create mode 100644 test/Microsoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/Utf8BufferTextReaderTests.cs diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HandshakeProtocol.cs b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HandshakeProtocol.cs index bc6168be43..8efbee6a26 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HandshakeProtocol.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/HandshakeProtocol.cs @@ -58,7 +58,8 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol private static JsonTextReader CreateJsonTextReader(ReadOnlyMemory payload) { - var textReader = new Utf8BufferTextReader(payload); + var textReader = new Utf8BufferTextReader(); + textReader.SetBuffer(payload); var reader = new JsonTextReader(textReader); reader.ArrayPool = JsonArrayPool.Shared; diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs index ebd9f86d4d..80d7e56440 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs @@ -58,11 +58,19 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol { while (TextMessageParser.TryParseMessage(ref input, out var payload)) { - var textReader = new Utf8BufferTextReader(payload); - var message = ParseMessage(textReader, binder); - if (message != null) + var textReader = Utf8BufferTextReader.Get(payload); + + try { - messages.Add(message); + var message = ParseMessage(textReader, binder); + if (message != null) + { + messages.Add(message); + } + } + finally + { + Utf8BufferTextReader.Return(textReader); } } @@ -103,6 +111,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol using (var reader = new JsonTextReader(textReader)) { reader.ArrayPool = JsonArrayPool.Shared; + reader.CloseInput = false; JsonUtils.CheckRead(reader); diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/Utf8BufferTextReader.cs b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/Utf8BufferTextReader.cs index f09e475fb6..227f74fc98 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/Utf8BufferTextReader.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/Utf8BufferTextReader.cs @@ -11,10 +11,54 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol internal class Utf8BufferTextReader : TextReader { private ReadOnlyMemory _utf8Buffer; + private Decoder _decoder; - public Utf8BufferTextReader(ReadOnlyMemory utf8Buffer) + [ThreadStatic] + private static Utf8BufferTextReader _cachedInstance; + +#if DEBUG + private bool _inUse; +#endif + + public Utf8BufferTextReader() + { + _decoder = Encoding.UTF8.GetDecoder(); + } + + public static Utf8BufferTextReader Get(ReadOnlyMemory utf8Buffer) + { + var reader = _cachedInstance; + if (reader == null) + { + reader = new Utf8BufferTextReader(); + } + + // Taken off the the thread static + _cachedInstance = null; +#if DEBUG + if (reader._inUse) + { + throw new InvalidOperationException("The reader wasn't returned!"); + } + + reader._inUse = true; +#endif + reader.SetBuffer(utf8Buffer); + return reader; + } + + public static void Return(Utf8BufferTextReader reader) + { + _cachedInstance = reader; +#if DEBUG + reader._inUse = false; +#endif + } + + public void SetBuffer(ReadOnlyMemory utf8Buffer) { _utf8Buffer = utf8Buffer; + _decoder.Reset(); } public override int Read(char[] buffer, int index, int count) @@ -25,33 +69,24 @@ namespace Microsoft.AspNetCore.SignalR.Internal.Protocol } var source = _utf8Buffer.Span; - var destination = new Span(buffer, index, count); - var destinationBytesCount = Encoding.UTF8.GetByteCount(buffer, index, count); - - // We have then the destination - if (source.Length > destinationBytesCount) - { - source = source.Slice(0, destinationBytesCount); - - _utf8Buffer = _utf8Buffer.Slice(destinationBytesCount); - } - else - { - _utf8Buffer = ReadOnlyMemory.Empty; - } - + var bytesUsed = 0; + var charsUsed = 0; #if NETCOREAPP2_1 - return Encoding.UTF8.GetChars(source, destination); + var destination = new Span(buffer, index, count); + _decoder.Convert(source, destination, false, out bytesUsed, out charsUsed, out var completed); #else unsafe { - fixed (char* destinationChars = &MemoryMarshal.GetReference(destination)) + fixed (char* destinationChars = &buffer[index]) fixed (byte* sourceBytes = &MemoryMarshal.GetReference(source)) { - return Encoding.UTF8.GetChars(sourceBytes, source.Length, destinationChars, destination.Length); + _decoder.Convert(sourceBytes, source.Length, destinationChars, count, false, out bytesUsed, out charsUsed, out var completed); } } #endif + _utf8Buffer = _utf8Buffer.Slice(bytesUsed); + + return charsUsed; } } } diff --git a/src/Microsoft.AspNetCore.SignalR.Common/Properties/AssemblyInfo.cs b/src/Microsoft.AspNetCore.SignalR.Common/Properties/AssemblyInfo.cs index acee0ea403..92e52dceeb 100644 --- a/src/Microsoft.AspNetCore.SignalR.Common/Properties/AssemblyInfo.cs +++ b/src/Microsoft.AspNetCore.SignalR.Common/Properties/AssemblyInfo.cs @@ -3,4 +3,5 @@ using System.Runtime.CompilerServices; -[assembly: InternalsVisibleTo("Microsoft.AspNetCore.SignalR.Tests.Utils, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] \ No newline at end of file +[assembly: InternalsVisibleTo("Microsoft.AspNetCore.SignalR.Tests.Utils, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] +[assembly: InternalsVisibleTo("Microsoft.AspNetCore.SignalR.Common.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/Utf8BufferTextReaderTests.cs b/test/Microsoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/Utf8BufferTextReaderTests.cs new file mode 100644 index 0000000000..08f921b104 --- /dev/null +++ b/test/Microsoft.AspNetCore.SignalR.Common.Tests/Internal/Protocol/Utf8BufferTextReaderTests.cs @@ -0,0 +1,119 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Buffers; +using System.IO; +using System.Text; +using Microsoft.AspNetCore.SignalR.Internal.Protocol; +using Xunit; + +namespace Microsoft.AspNetCore.SignalR.Common.Tests.Internal.Protocol +{ + public class Utf8BufferTextReaderTests + { + [Fact] + public void ReadingWhenCharBufferBigEnough() + { + var buffer = Encoding.UTF8.GetBytes("Hello World"); + var reader = new Utf8BufferTextReader(); + reader.SetBuffer(buffer); + + var chars = new char[1024]; + int read = reader.Read(chars, 0, chars.Length); + + Assert.Equal("Hello World", new string(chars, 0, read)); + } + + [Fact] + public void ReadingUnicodeWhenCharBufferBigEnough() + { + var buffer = Encoding.UTF8.GetBytes("a\u00E4\u00E4\u00a9o"); + var reader = new Utf8BufferTextReader(); + reader.SetBuffer(buffer); + + var chars = new char[1024]; + int read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(5, read); + Assert.Equal("a\u00E4\u00E4\u00a9o", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(0, read); + } + + [Fact] + public void ReadingWhenCharBufferBigEnoughAndNotStartingFromZero() + { + var buffer = Encoding.UTF8.GetBytes("Hello World"); + var reader = new Utf8BufferTextReader(); + reader.SetBuffer(buffer); + + var chars = new char[1024]; + int read = reader.Read(chars, 10, chars.Length - 10); + + Assert.Equal(11, read); + Assert.Equal("Hello World", new string(chars, 10, read)); + } + + [Fact] + public void ReadingWhenBufferTooSmall() + { + var buffer = Encoding.UTF8.GetBytes("Hello World"); + var reader = new Utf8BufferTextReader(); + reader.SetBuffer(buffer); + + var chars = new char[5]; + int read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(5, read); + Assert.Equal("Hello", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(5, read); + Assert.Equal(" Worl", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(1, read); + Assert.Equal("d", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(0, read); + + read = reader.Read(chars, 0, 1); + + Assert.Equal(0, read); + } + + [Fact] + public void ReadingUnicodeWhenBufferTooSmall() + { + var buffer = Encoding.UTF8.GetBytes("\u00E4\u00E4\u00E5"); + var reader = new Utf8BufferTextReader(); + reader.SetBuffer(buffer); + + var chars = new char[2]; + int read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(2, read); + Assert.Equal("\u00E4\u00E4", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(1, read); + Assert.Equal("\u00E5", new string(chars, 0, read)); + + read = reader.Read(chars, 0, chars.Length); + + Assert.Equal(0, read); + + read = reader.Read(chars, 0, 1); + + Assert.Equal(0, read); + } + } +} \ No newline at end of file From 79b51ad642824952ff1abac058462f6652d89344 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 27 Mar 2018 12:57:13 +1300 Subject: [PATCH 5/6] Added logging of .NET client HTTP requests (#1723) --- .../HttpConnection.cs | 13 +++- .../Internal/LoggingHttpMessageHandler.cs | 62 +++++++++++++++++++ .../WebSocketsTransport.Log.cs | 10 +-- .../WebSocketsTransport.cs | 9 +-- .../HttpConnectionTests.cs | 40 ++++++++++++ 5 files changed, 122 insertions(+), 12 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/LoggingHttpMessageHandler.cs diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs index 3c6351e9a0..80d60342d2 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs @@ -14,6 +14,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Sockets.Client.Http; +using Microsoft.AspNetCore.Sockets.Client.Http.Internal; using Microsoft.AspNetCore.Sockets.Client.Internal; using Microsoft.AspNetCore.Sockets.Http.Internal; using Microsoft.AspNetCore.Sockets.Internal; @@ -26,7 +27,9 @@ namespace Microsoft.AspNetCore.Sockets.Client public partial class HttpConnection : IConnection { private static readonly TimeSpan HttpClientTimeout = TimeSpan.FromSeconds(120); +#if !NETCOREAPP2_1 private static readonly Version Windows8Version = new Version(6, 2); +#endif private readonly ILoggerFactory _loggerFactory; private readonly ILogger _logger; @@ -99,10 +102,11 @@ namespace Microsoft.AspNetCore.Sockets.Client private HttpClient CreateHttpClient() { - HttpMessageHandler httpMessageHandler = null; + var httpClientHandler = new HttpClientHandler(); + HttpMessageHandler httpMessageHandler = httpClientHandler; + if (_httpOptions != null) { - var httpClientHandler = new HttpClientHandler(); if (_httpOptions.Proxy != null) { httpClientHandler.Proxy = _httpOptions.Proxy; @@ -135,7 +139,10 @@ namespace Microsoft.AspNetCore.Sockets.Client } } - var httpClient = httpMessageHandler == null ? new HttpClient() : new HttpClient(httpMessageHandler); + // Wrap message handler in a logging handler last to ensure it is always present + httpMessageHandler = new LoggingHttpMessageHandler(httpMessageHandler, _loggerFactory); + + var httpClient = new HttpClient(httpMessageHandler); httpClient.Timeout = HttpClientTimeout; return httpClient; diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/LoggingHttpMessageHandler.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/LoggingHttpMessageHandler.cs new file mode 100644 index 0000000000..4528821cb4 --- /dev/null +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/LoggingHttpMessageHandler.cs @@ -0,0 +1,62 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Microsoft.AspNetCore.Sockets.Client.Http.Internal +{ + public class LoggingHttpMessageHandler : DelegatingHandler + { + private readonly ILogger _logger; + + public LoggingHttpMessageHandler(HttpMessageHandler inner, ILoggerFactory loggerFactory) : base(inner) + { + if (loggerFactory == null) + { + throw new ArgumentNullException(nameof(loggerFactory)); + } + + _logger = loggerFactory.CreateLogger(); + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + Log.SendingHttpRequest(_logger, request.RequestUri); + + var response = await base.SendAsync(request, cancellationToken); + + if (!response.IsSuccessStatusCode) + { + Log.UnsuccessfulHttpResponse(_logger, request.RequestUri, response.StatusCode); + } + + return response; + } + + private static class Log + { + private static readonly Action _sendingHttpRequest = + LoggerMessage.Define(LogLevel.Trace, new EventId(1, "SendingHttpRequest"), "Sending HTTP request to '{RequestUrl}'."); + + private static readonly Action _unsuccessfulHttpResponse = + LoggerMessage.Define(LogLevel.Warning, new EventId(2, "UnsuccessfulHttpResponse"), "Unsuccessful HTTP response status code of {StatusCode} return from '{RequestUrl}'."); + + public static void SendingHttpRequest(ILogger logger, Uri requestUrl) + { + _sendingHttpRequest(logger, requestUrl, null); + } + public static void UnsuccessfulHttpResponse(ILogger logger, Uri requestUrl, HttpStatusCode statusCode) + { + _unsuccessfulHttpResponse(logger, requestUrl, statusCode, null); + } + } + } +} diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.Log.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.Log.cs index 4d06ca2412..062c6c9a0f 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.Log.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.Log.cs @@ -12,8 +12,8 @@ namespace Microsoft.AspNetCore.Sockets.Client { private static class Log { - private static readonly Action _startTransport = - LoggerMessage.Define(LogLevel.Information, new EventId(1, "StartTransport"), "Starting transport. Transfer mode: {TransferFormat}."); + private static readonly Action _startTransport = + LoggerMessage.Define(LogLevel.Information, new EventId(1, "StartTransport"), "Starting transport. Transfer mode: {TransferFormat}. Url: '{WebSocketUrl}'."); private static readonly Action _transportStopped = LoggerMessage.Define(LogLevel.Debug, new EventId(2, "TransportStopped"), "Transport stopped."); @@ -43,7 +43,7 @@ namespace Microsoft.AspNetCore.Sockets.Client LoggerMessage.Define(LogLevel.Debug, new EventId(10, "MessageToApp"), "Passing message to application. Payload size: {Count}."); private static readonly Action _webSocketClosed = - LoggerMessage.Define(LogLevel.Information, new EventId(11, "WebSocketClosed"), "Websocket closed by the server. Close status {CloseStatus}."); + LoggerMessage.Define(LogLevel.Information, new EventId(11, "WebSocketClosed"), "WebSocket closed by the server. Close status {CloseStatus}."); private static readonly Action _messageReceived = LoggerMessage.Define(LogLevel.Debug, new EventId(12, "MessageReceived"), "Message received. Type: {MessageType}, size: {Count}, EndOfMessage: {EndOfMessage}."); @@ -66,9 +66,9 @@ namespace Microsoft.AspNetCore.Sockets.Client private static readonly Action _cancelMessage = LoggerMessage.Define(LogLevel.Debug, new EventId(18, "CancelMessage"), "Canceled passing message to application."); - public static void StartTransport(ILogger logger, TransferFormat transferFormat) + public static void StartTransport(ILogger logger, TransferFormat transferFormat, Uri webSocketUrl) { - _startTransport(logger, transferFormat, null); + _startTransport(logger, transferFormat, webSocketUrl, null); } public static void TransportStopped(ILogger logger, Exception exception) diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.cs index e54e4d99d9..8e3981be43 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/WebSocketsTransport.cs @@ -109,10 +109,11 @@ namespace Microsoft.AspNetCore.Sockets.Client ? WebSocketMessageType.Binary : WebSocketMessageType.Text; + var resolvedUrl = ResolveWebSocketsUrl(url); - Log.StartTransport(_logger, transferFormat); + Log.StartTransport(_logger, transferFormat, resolvedUrl); - await Connect(url); + await _webSocket.ConnectAsync(resolvedUrl, CancellationToken.None); // TODO: Handle TCP connection errors // https://github.com/SignalR/SignalR/blob/1fba14fa3437e24c204dfaf8a18db3fce8acad3c/src/Microsoft.AspNet.SignalR.Core/Owin/WebSockets/WebSocketHandler.cs#L248-L251 @@ -324,7 +325,7 @@ namespace Microsoft.AspNetCore.Sockets.Client ws.State == WebSocketState.CloseSent); } - private async Task Connect(Uri url) + private static Uri ResolveWebSocketsUrl(Uri url) { var uriBuilder = new UriBuilder(url); if (url.Scheme == "http") @@ -336,7 +337,7 @@ namespace Microsoft.AspNetCore.Sockets.Client uriBuilder.Scheme = "wss"; } - await _webSocket.ConnectAsync(uriBuilder.Uri, CancellationToken.None); + return uriBuilder.Uri; } public async Task StopAsync() diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.cs index 613b892ffb..27227c1b4a 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.cs @@ -12,7 +12,9 @@ using Microsoft.AspNetCore.Client.Tests; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Sockets.Client; using Microsoft.AspNetCore.Sockets.Client.Http; +using Microsoft.AspNetCore.Sockets.Client.Http.Internal; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Logging.Testing; using Moq; using Xunit; @@ -172,5 +174,43 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests Assert.Same(httpOptions.Proxy, httpClientHandler.Proxy); Assert.Same(httpOptions.Credentials, httpClientHandler.Credentials); } + + [Fact] + public async Task HttpRequestAndErrorResponseLogged() + { + var testHttpHandler = new TestHttpMessageHandler(false); + + testHttpHandler.OnNegotiate((request, cancellationToken) => ResponseUtils.CreateResponse(HttpStatusCode.BadGateway)); + + var httpOptions = new HttpOptions(); + httpOptions.HttpMessageHandler = inner => testHttpHandler; + + const string loggerName = "Microsoft.AspNetCore.Sockets.Client.Http.Internal.LoggingHttpMessageHandler"; + var testSink = new TestSink(); + var logger = new TestLogger(loggerName, testSink, true); + + var mockLoggerFactory = new Mock(); + mockLoggerFactory + .Setup(m => m.CreateLogger(It.IsAny())) + .Returns((string categoryName) => (categoryName == loggerName) ? (ILogger)logger : NullLogger.Instance); + + try + { + await WithConnectionAsync( + CreateConnection(httpOptions, loggerFactory: mockLoggerFactory.Object, url: "http://fakeuri.org/"), + async (connection, closed) => + { + await connection.StartAsync(TransferFormat.Text).OrTimeout(); + }); + } + catch + { + // ignore connection error + } + + Assert.Equal(2, testSink.Writes.Count); + Assert.Equal("SendingHttpRequest", testSink.Writes[0].EventId.Name); + Assert.Equal("UnsuccessfulHttpResponse", testSink.Writes[1].EventId.Name); + } } } From cc52beec1734981583ce2885973eaa9fbc51f6c2 Mon Sep 17 00:00:00 2001 From: Mikael Mengistu Date: Tue, 27 Mar 2018 00:18:36 +0000 Subject: [PATCH 6/6] Fallback for TS client --- .../FunctionalTests/ts/HubConnectionTests.ts | 27 ++++++++ .../ts/signalr/spec/HttpConnection.spec.ts | 18 +++++ clients/ts/signalr/src/HttpConnection.ts | 65 +++++++++++++------ 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/clients/ts/FunctionalTests/ts/HubConnectionTests.ts b/clients/ts/FunctionalTests/ts/HubConnectionTests.ts index 0ae87d7de0..faff3b1fab 100644 --- a/clients/ts/FunctionalTests/ts/HubConnectionTests.ts +++ b/clients/ts/FunctionalTests/ts/HubConnectionTests.ts @@ -544,6 +544,33 @@ describe("hubConnection", () => { } }); + it("transport falls back from WebSockets to SSE or LongPolling", async (done) => { + // Replace Websockets with a function that just + // throws to force fallback. + const oldWebSocket = (window as any).WebSocket; + (window as any).WebSocket = () => { + throw new Error("Kick rocks"); + }; + + const hubConnection = new HubConnection(TESTHUBENDPOINT_URL, { + logger: LogLevel.Trace, + protocol: new JsonHubProtocol(), + }); + + try { + await hubConnection.start(); + + // Make sure that we connect with SSE or LongPolling after Websockets fail + const transportName = await hubConnection.invoke("GetActiveTransportName"); + expect(transportName === "ServerSentEvents" || transportName === "LongPolling").toBe(true); + } catch (e) { + fail(e); + } finally { + (window as any).WebSocket = oldWebSocket; + done(); + } + }); + function getJwtToken(url): Promise { return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); diff --git a/clients/ts/signalr/spec/HttpConnection.spec.ts b/clients/ts/signalr/spec/HttpConnection.spec.ts index 1cd521798f..3d06eaf8de 100644 --- a/clients/ts/signalr/spec/HttpConnection.spec.ts +++ b/clients/ts/signalr/spec/HttpConnection.spec.ts @@ -134,6 +134,24 @@ describe("HttpConnection", () => { done(); }); + it("start throws after all transports fail", async (done) => { + const options: IHttpConnectionOptions = { + httpClient: new TestHttpClient() + .on("POST", (r) => ({ connectionId: "42", availableTransports: [] })) + .on("GET", (r) => { throw new Error("fail"); }), + } as IHttpConnectionOptions; + + const connection = new HttpConnection("http://tempuri.org?q=myData", options); + try { + await connection.start(TransferFormat.Text); + fail(); + done(); + } catch (e) { + expect(e.message).toBe("Unable to initialize any of the available transports."); + } + done(); + }); + it("preserves user's query string", async (done) => { let connectUrl: string; const fakeTransport: ITransport = { diff --git a/clients/ts/signalr/src/HttpConnection.ts b/clients/ts/signalr/src/HttpConnection.ts index 614e51b416..febd61c21c 100644 --- a/clients/ts/signalr/src/HttpConnection.ts +++ b/clients/ts/signalr/src/HttpConnection.ts @@ -79,39 +79,29 @@ export class HttpConnection implements IConnection { // No need to add a connection ID in this case this.url = this.baseUrl; this.transport = this.constructTransport(TransportType.WebSockets); + // We should just call connect directly in this case. + // No fallback or negotiate in this case. + await this.transport.connect(this.url, transferFormat, this); } else { - let headers; const token = this.options.accessTokenFactory(); + let headers; if (token) { headers = { ["Authorization"]: `Bearer ${token}`, }; } - const negotiatePayload = await this.httpClient.post(this.resolveNegotiateUrl(this.baseUrl), { - content: "", - headers, - }); - - const negotiateResponse: INegotiateResponse = JSON.parse(negotiatePayload.content as string); - this.connectionId = negotiateResponse.connectionId; - + const negotiateResponse = await this.getNegotiationResponse(headers); // the user tries to stop the the connection when it is being started if (this.connectionState === ConnectionState.Disconnected) { return; } - - if (this.connectionId) { - this.url = this.baseUrl + (this.baseUrl.indexOf("?") === -1 ? "?" : "&") + `id=${this.connectionId}`; - this.transport = this.createTransport(this.options.transport, negotiateResponse.availableTransports, transferFormat); - } + await this.createTransport(this.options.transport, negotiateResponse, transferFormat, headers); } this.transport.onreceive = this.onreceive; this.transport.onclose = (e) => this.stopConnection(true, e); - await this.transport.connect(this.url, transferFormat, this); - // only change the state if we were connecting to not overwrite // the state if the connection is already marked as Disconnected this.changeState(ConnectionState.Connecting, ConnectionState.Connected); @@ -123,16 +113,51 @@ export class HttpConnection implements IConnection { } } - private createTransport(requestedTransport: TransportType | ITransport, availableTransports: IAvailableTransport[], requestedTransferFormat: TransferFormat): ITransport { + private async getNegotiationResponse(headers: any): Promise { + const response = await this.httpClient.post(this.resolveNegotiateUrl(this.baseUrl), { + content: "", + headers, + }); + return JSON.parse(response.content as string); + } + + private updateConnectionId(negotiateResponse: INegotiateResponse) { + this.connectionId = negotiateResponse.connectionId; + this.url = this.baseUrl + (this.baseUrl.indexOf("?") === -1 ? "?" : "&") + `id=${this.connectionId}`; + } + + private async createTransport(requestedTransport: TransportType | ITransport, negotiateResponse: INegotiateResponse, requestedTransferFormat: TransferFormat, headers: any): Promise { + this.updateConnectionId(negotiateResponse); if (this.isITransport(requestedTransport)) { this.logger.log(LogLevel.Trace, "Connection was provided an instance of ITransport, using that directly."); - return requestedTransport; + this.transport = requestedTransport; + await this.transport.connect(this.url, requestedTransferFormat, this); + + // only change the state if we were connecting to not overwrite + // the state if the connection is already marked as Disconnected + this.changeState(ConnectionState.Connecting, ConnectionState.Connected); + return; } - for (const endpoint of availableTransports) { + const transports = negotiateResponse.availableTransports; + for (const endpoint of transports) { + this.connectionState = ConnectionState.Connecting; const transport = this.resolveTransport(endpoint, requestedTransport, requestedTransferFormat); if (typeof transport === "number") { - return this.constructTransport(transport); + this.transport = this.constructTransport(transport); + if (negotiateResponse.connectionId === null) { + negotiateResponse = await this.getNegotiationResponse(headers); + this.updateConnectionId(negotiateResponse); + } + try { + await this.transport.connect(this.url, requestedTransferFormat, this); + this.changeState(ConnectionState.Connecting, ConnectionState.Connected); + return; + } catch (ex) { + this.logger.log(LogLevel.Error, `Failed to start the transport' ${TransportType[transport]}:' transport'${ex}'`); + this.connectionState = ConnectionState.Disconnected; + negotiateResponse.connectionId = null; + } } }