fix #13651 by logging serialization failures as error (#20116)

src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs

@@ -1335,6 +1335,102 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests
             }
         }
 
+        [Theory]
+        [QuarantinedTest]
+        [MemberData(nameof(HubProtocolsList))]
+        public async Task ServerLogsErrorIfClientInvokeCannotBeSerialized(string protocolName)
+        {
+            // Just to help sanity check that the right exception is thrown
+            var exceptionSubstring = protocolName switch
+            {
+                "json" => "A possible object cycle was detected.",
+                "newtonsoft-json" => "A possible object cycle was detected.",
+                "messagepack" => "Failed to serialize Microsoft.AspNetCore.SignalR.Client.FunctionalTests.TestHub+Unserializable value.",
+                var x => throw new Exception($"The test does not have an exception string for the protocol '{x}'!"),
+            };
+
+            var protocol = HubProtocols[protocolName];
+            using (var server = await StartServer<Startup>(write => write.EventId.Name == "FailedWritingMessage"))
+            {
+                var connection = CreateHubConnection(server.Url, "/default", HttpTransportType.WebSockets, protocol, LoggerFactory);
+                var closedTcs = new TaskCompletionSource<Exception>(TaskCreationOptions.RunContinuationsAsynchronously);
+                connection.Closed += (ex) => { closedTcs.TrySetResult(ex); return Task.CompletedTask; };
+                try
+                {
+                    await connection.StartAsync().OrTimeout();
+
+                    var result = connection.InvokeAsync<string>(nameof(TestHub.CallWithUnserializableObject));
+
+                    // The connection should close.
+                    Assert.Null(await closedTcs.Task.OrTimeout());
+
+                    await Assert.ThrowsAsync<TaskCanceledException>(() => result).OrTimeout();
+                }
+                catch (Exception ex)
+                {
+                    LoggerFactory.CreateLogger<HubConnectionTests>().LogError(ex, "{ExceptionType} from test", ex.GetType().FullName);
+                    throw;
+                }
+                finally
+                {
+                    await connection.DisposeAsync().OrTimeout();
+                }
+
+                var errorLog = server.GetLogs().SingleOrDefault(r => r.Write.EventId.Name == "FailedWritingMessage");
+                Assert.NotNull(errorLog);
+                Assert.Contains(exceptionSubstring, errorLog.Write.Exception.Message);
+                Assert.Equal(LogLevel.Error, errorLog.Write.LogLevel);
+            }
+        }
+
+        [Theory]
+        [QuarantinedTest]
+        [MemberData(nameof(HubProtocolsList))]
+        public async Task ServerLogsErrorIfReturnValueCannotBeSerialized(string protocolName)
+        {
+            // Just to help sanity check that the right exception is thrown
+            var exceptionSubstring = protocolName switch
+            {
+                "json" => "A possible object cycle was detected.",
+                "newtonsoft-json" => "A possible object cycle was detected.",
+                "messagepack" => "Failed to serialize Microsoft.AspNetCore.SignalR.Client.FunctionalTests.TestHub+Unserializable value.",
+                var x => throw new Exception($"The test does not have an exception string for the protocol '{x}'!"),
+            };
+
+            var protocol = HubProtocols[protocolName];
+            using (var server = await StartServer<Startup>(write => write.EventId.Name == "FailedWritingMessage"))
+            {
+                var connection = CreateHubConnection(server.Url, "/default", HttpTransportType.LongPolling, protocol, LoggerFactory);
+                var closedTcs = new TaskCompletionSource<Exception>(TaskCreationOptions.RunContinuationsAsynchronously);
+                connection.Closed += (ex) => { closedTcs.TrySetResult(ex); return Task.CompletedTask; };
+                try
+                {
+                    await connection.StartAsync().OrTimeout();
+
+                    var result = connection.InvokeAsync<string>(nameof(TestHub.GetUnserializableObject)).OrTimeout();
+
+                    // The connection should close.
+                    Assert.Null(await closedTcs.Task.OrTimeout());
+
+                    await Assert.ThrowsAsync<TaskCanceledException>(() => result).OrTimeout();
+                }
+                catch (Exception ex)
+                {
+                    LoggerFactory.CreateLogger<HubConnectionTests>().LogError(ex, "{ExceptionType} from test", ex.GetType().FullName);
+                    throw;
+                }
+                finally
+                {
+                    await connection.DisposeAsync().OrTimeout();
+                }
+
+                var errorLog = server.GetLogs().SingleOrDefault(r => r.Write.EventId.Name == "FailedWritingMessage");
+                Assert.NotNull(errorLog);
+                Assert.Contains(exceptionSubstring, errorLog.Write.Exception.Message);
+                Assert.Equal(LogLevel.Error, errorLog.Write.LogLevel);
+            }
+        }
+
         [Fact]
         public async Task RandomGenericIsNotTreatedAsStream()
         {

src/SignalR/clients/csharp/Client/test/FunctionalTests/Hubs.cs

@@ -95,6 +95,33 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests
         {
             return Context.Features.Get<IHttpTransportFeature>().TransportType.ToString();
         }
+
+        public async Task CallWithUnserializableObject()
+        {
+            await Clients.All.SendAsync("Foo", Unserializable.Create());
+        }
+
+        public Unserializable GetUnserializableObject()
+        {
+            return Unserializable.Create();
+        }
+
+        public class Unserializable
+        {
+            public Unserializable Child { get; private set; }
+
+            private Unserializable()
+            {
+            }
+
+            internal static Unserializable Create()
+            {
+                // Loops throw off every serializer ;).
+                var o = new Unserializable();
+                o.Child = o;
+                return o;
+            }
+        }
     }
 
     public class DynamicTestHub : DynamicHub

src/SignalR/common/testassets/Tests.Utils/InProcessTestServer.cs

@@ -84,6 +84,8 @@ namespace Microsoft.AspNetCore.SignalR.Tests
             _logger = _loggerFactory.CreateLogger<InProcessTestServer<TStartup>>();
         }
 
+        public IList<LogRecord> GetLogs() => _logSinkProvider.GetLogs();
+
         private async Task StartServerInner()
         {
             // We're using 127.0.0.1 instead of localhost to ensure that we use IPV4 across different OSes

src/SignalR/common/testassets/Tests.Utils/LogRecord.cs

@@ -7,7 +7,7 @@ using Microsoft.Extensions.Logging.Testing;
 namespace Microsoft.AspNetCore.SignalR.Tests
 {
     // WriteContext, but with a timestamp...
-    internal class LogRecord
+    public class LogRecord
     {
         public DateTime Timestamp { get; }
         public WriteContext Write { get; }

src/SignalR/server/Core/src/HubConnectionContext.cs

@@ -689,7 +689,7 @@ namespace Microsoft.AspNetCore.SignalR
                 LoggerMessage.Define(LogLevel.Debug, new EventId(5, "HandshakeFailed"), "Failed connection handshake.");
 
             private static readonly Action<ILogger, Exception> _failedWritingMessage =
-                LoggerMessage.Define(LogLevel.Debug, new EventId(6, "FailedWritingMessage"), "Failed writing message. Aborting connection.");
+                LoggerMessage.Define(LogLevel.Error, new EventId(6, "FailedWritingMessage"), "Failed writing message. Aborting connection.");
 
             private static readonly Action<ILogger, string, int, Exception> _protocolVersionFailed =
                 LoggerMessage.Define<string, int>(LogLevel.Debug, new EventId(7, "ProtocolVersionFailed"), "Server does not support version {Version} of the {Protocol} protocol.");

src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs

@@ -3299,7 +3299,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests
         [Fact]
         public async Task ConnectionAbortedIfSendFailsWithProtocolError()
         {
-            using (StartVerifiableLog())
+            using (StartVerifiableLog(write => write.EventId.Name == "FailedWritingMessage"))
             {
                 var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services =>
                 {