From e31fc1e57d1c3262a4764268de61e5e59273b4e6 Mon Sep 17 00:00:00 2001 From: Pawel Kadluczka Date: Fri, 26 May 2017 07:36:34 -0700 Subject: [PATCH] Closing transports with error if hubs cannot be created - Preventing from closing long polling transport with 204 in case of error - Reporting an error to the client if WebSocket was not closed normally Note in case of ServerSentEvents it is not possible on the client to tell the difference between when the server closed event stream due to an exception or because the client left OnConnectedAsync. In both cases the client sees only that the stream was closed. Part of: #163 --- .../Transports.ts | 2 +- .../Startup.cs | 6 +- .../UncreatableHub.cs | 14 ++++ .../wwwroot/js/hubConnectionTests.js | 16 ++++ .../ServerSentEventsTransport.cs | 1 + .../WebSocketsTransport.cs | 6 +- .../HttpConnectionDispatcher.cs | 2 +- .../Transports/LongPollingTransport.cs | 6 ++ .../Transports/ServerSentEventsTransport.cs | 2 + .../EndToEndTests.cs | 74 ++++++++++++++++--- .../ServerFixture.cs | 1 + .../UncreatableHub.cs | 12 +++ .../HttpConnectionDispatcherTests.cs | 23 ------ 13 files changed, 125 insertions(+), 40 deletions(-) create mode 100644 client-ts/Microsoft.AspNetCore.SignalR.Test.Server/UncreatableHub.cs create mode 100644 test/Microsoft.AspNetCore.SignalR.Tests/UncreatableHub.cs diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Transports.ts b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Transports.ts index fca859aec8..ab3a428d86 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Transports.ts +++ b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Transports.ts @@ -125,7 +125,7 @@ export class ServerSentEventsTransport implements ITransport { // don't report an error if the transport did not start successfully if (this.eventSource && this.onClosed) { - this.onClosed(new Error(e.message)); + this.onClosed(new Error(e.message || "Error occurred")); } } diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/Startup.cs b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/Startup.cs index cb5ca7f04d..6137826907 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/Startup.cs +++ b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/Startup.cs @@ -25,10 +25,8 @@ namespace Microsoft.AspNetCore.SignalR.Test.Server app.UseFileServer(); app.UseSockets(options => options.MapEndPoint("echo")); - app.UseSignalR(routes => - { - routes.MapHub("testhub"); - }); + app.UseSignalR(options => options.MapHub("testhub")); + app.UseSignalR(options => options.MapHub("uncreatable")); } } } diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/UncreatableHub.cs b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/UncreatableHub.cs new file mode 100644 index 0000000000..17bb96c82a --- /dev/null +++ b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/UncreatableHub.cs @@ -0,0 +1,14 @@ +// 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; + +namespace Microsoft.AspNetCore.SignalR.Test.Server +{ + public class UncreatableHub: Hub + { + public UncreatableHub(object obj) + { + } + } +} diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/wwwroot/js/hubConnectionTests.js b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/wwwroot/js/hubConnectionTests.js index 0d90a2d066..eea1907676 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/wwwroot/js/hubConnectionTests.js +++ b/client-ts/Microsoft.AspNetCore.SignalR.Test.Server/wwwroot/js/hubConnectionTests.js @@ -142,5 +142,21 @@ describe('hubConnection', () => { }); }); }); + + it(`over ${signalR.TransportType[transportType]} closed with error if hub cannot be created`, done =>{ + let errorRegex = { + WebSockets: "1011", // Message is browser specific (e.g. 'Websocket closed with status code: 1011') + LongPolling: "Status: 500", + ServerSentEvents: "Error occurred" + }; + + let hubConnection = new signalR.HubConnection(`http://${document.location.host}/uncreatable`, 'formatType=json&format=text'); + + hubConnection.onClosed = error => { + expect(error).toMatch(errorRegex[signalR.TransportType[transportType]]); + done(); + } + hubConnection.start(transportType) + }); }); }); diff --git a/src/Microsoft.AspNetCore.Sockets.Client/ServerSentEventsTransport.cs b/src/Microsoft.AspNetCore.Sockets.Client/ServerSentEventsTransport.cs index cd4e1f2ce9..3b5ac78a8a 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client/ServerSentEventsTransport.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client/ServerSentEventsTransport.cs @@ -114,6 +114,7 @@ namespace Microsoft.AspNetCore.Sockets.Client finally { _transportCts.Cancel(); + stream.Dispose(); } } diff --git a/src/Microsoft.AspNetCore.Sockets.Client/WebSocketsTransport.cs b/src/Microsoft.AspNetCore.Sockets.Client/WebSocketsTransport.cs index 16cf6b93d3..c52caa1a91 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client/WebSocketsTransport.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client/WebSocketsTransport.cs @@ -84,7 +84,11 @@ namespace Microsoft.AspNetCore.Sockets.Client { _logger.LogInformation("Websocket closed by the server. Close status {0}", receiveResult.CloseStatus); - _application.Output.Complete(); + _application.Output.Complete( + receiveResult.CloseStatus == WebSocketCloseStatus.NormalClosure + ? null + : new InvalidOperationException( + $"Websocket closed with error: {receiveResult.CloseStatus}.")); return; } diff --git a/src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs b/src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs index 55ce652fb7..9a08ea6984 100644 --- a/src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs +++ b/src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs @@ -202,7 +202,7 @@ namespace Microsoft.AspNetCore.Sockets var pollAgain = true; - // If the application ended before the transport task then we need to potentially need to end the + // If the application ended before the transport task then we need to potentially need to end the // connection if (resultTask == state.ApplicationTask) { diff --git a/src/Microsoft.AspNetCore.Sockets.Http/Transports/LongPollingTransport.cs b/src/Microsoft.AspNetCore.Sockets.Http/Transports/LongPollingTransport.cs index b1b0efab9f..a70db311a2 100644 --- a/src/Microsoft.AspNetCore.Sockets.Http/Transports/LongPollingTransport.cs +++ b/src/Microsoft.AspNetCore.Sockets.Http/Transports/LongPollingTransport.cs @@ -32,6 +32,7 @@ namespace Microsoft.AspNetCore.Sockets.Transports { if (!await _application.WaitToReadAsync(token)) { + await _application.Completion; _logger.LogInformation("Terminating Long Polling connection by sending 204 response."); context.Response.StatusCode = StatusCodes.Status204NoContent; return; @@ -82,6 +83,11 @@ namespace Microsoft.AspNetCore.Sockets.Transports // The background thread will eventually dispose this connection if it's inactive _logger.LogDebug("Client disconnected from Long Polling endpoint."); } + catch (Exception ex) + { + _logger.LogError(ex, "Long Polling transport was terminated due to an error"); + throw; + } } } } diff --git a/src/Microsoft.AspNetCore.Sockets.Http/Transports/ServerSentEventsTransport.cs b/src/Microsoft.AspNetCore.Sockets.Http/Transports/ServerSentEventsTransport.cs index cdd4b020f7..1cc8e0ff44 100644 --- a/src/Microsoft.AspNetCore.Sockets.Http/Transports/ServerSentEventsTransport.cs +++ b/src/Microsoft.AspNetCore.Sockets.Http/Transports/ServerSentEventsTransport.cs @@ -65,6 +65,8 @@ namespace Microsoft.AspNetCore.Sockets.Transports await output.FlushAsync(); } } + + await _application.Completion; } catch (OperationCanceledException) { diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs index e652360531..73b2bb912d 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs @@ -3,20 +3,21 @@ using System; using System.Collections.Generic; +using System.Net.Http; using System.Net.WebSockets; using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNetCore.SignalR.Client; +using Microsoft.AspNetCore.SignalR.Tests.Common; using Microsoft.AspNetCore.Sockets; using Microsoft.AspNetCore.Sockets.Client; using Microsoft.AspNetCore.Testing.xunit; using Microsoft.Extensions.Logging; -using Xunit; - -using ClientConnection = Microsoft.AspNetCore.Sockets.Client.Connection; -using Microsoft.AspNetCore.SignalR.Tests.Common; -using Xunit.Abstractions; using Microsoft.Extensions.Logging.Testing; +using Xunit; +using Xunit.Abstractions; +using ClientConnection = Microsoft.AspNetCore.Sockets.Client.Connection; namespace Microsoft.AspNetCore.SignalR.Tests { @@ -85,9 +86,8 @@ namespace Microsoft.AspNetCore.SignalR.Tests var logger = loggerFactory.CreateLogger(); const string message = "Major Key"; - var baseUrl = _serverFixture.BaseUrl; - string url = baseUrl + "/echo"; + var url = _serverFixture.BaseUrl + "/echo"; var connection = new ClientConnection(new Uri(url), loggerFactory); try { @@ -157,9 +157,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests { var logger = loggerFactory.CreateLogger(); - var baseUrl = _serverFixture.BaseUrl; - - string url = baseUrl + "/echo"; + var url = _serverFixture.BaseUrl + "/echo"; var connection = new ClientConnection(new Uri(url), loggerFactory); try { @@ -195,6 +193,62 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [ConditionalFact] + [OSSkipCondition(OperatingSystems.Windows, WindowsVersions.Win7, WindowsVersions.Win2008R2, SkipReason = "No WebSockets Client for this platform")] + public async Task ServerClosesConnectionWithErrorIfHubCannotBeCreated_WebSocket() + { + var exception = await Assert.ThrowsAsync( + async () => await ServerClosesConnectionWithErrorIfHubCannotBeCreated(TransportType.WebSockets)); + Assert.Equal("Websocket closed with error: InternalServerError.", exception.Message); + } + + [Fact] + public async Task ServerClosesConnectionWithErrorIfHubCannotBeCreated_LongPolling() + { + var exception = await Assert.ThrowsAsync( + async () => await ServerClosesConnectionWithErrorIfHubCannotBeCreated(TransportType.LongPolling)); + Assert.Equal("Response status code does not indicate success: 500 (Internal Server Error).", exception.Message); + } + + private async Task ServerClosesConnectionWithErrorIfHubCannotBeCreated(TransportType transportType) + { + using (StartLog(out var loggerFactory, testName: $"ConnectionCanSendAndReceiveMessages_{transportType.ToString()}")) + { + var logger = loggerFactory.CreateLogger(); + + var url = _serverFixture.BaseUrl + "/uncreatable"; + var connection = new HubConnection(new Uri(url), loggerFactory); + try + { + var closeTcs = new TaskCompletionSource(); + + connection.Closed += e => + { + logger.LogInformation("Connection closed"); + if (e != null) + { + closeTcs.TrySetException(e); + } + else + { + closeTcs.TrySetResult(null); + } + }; + + logger.LogInformation("Starting connection to {url}", url); + await connection.StartAsync(transportType).OrTimeout(); + + await closeTcs.Task.OrTimeout(); + } + finally + { + logger.LogInformation("Disposing Connection"); + await connection.DisposeAsync().OrTimeout(); + logger.LogInformation("Disposed Connection"); + } + } + } + public static IEnumerable TransportTypes() => new[] { diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/ServerFixture.cs b/test/Microsoft.AspNetCore.SignalR.Tests/ServerFixture.cs index 5fa3d2d629..59d86aa51f 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/ServerFixture.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/ServerFixture.cs @@ -47,6 +47,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests public void Configure(IApplicationBuilder app, IHostingEnvironment env) { app.UseSockets(options => options.MapEndPoint("echo")); + app.UseSignalR(options => options.MapHub("uncreatable")); } } diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/UncreatableHub.cs b/test/Microsoft.AspNetCore.SignalR.Tests/UncreatableHub.cs new file mode 100644 index 0000000000..97142cf271 --- /dev/null +++ b/test/Microsoft.AspNetCore.SignalR.Tests/UncreatableHub.cs @@ -0,0 +1,12 @@ +// 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. + +namespace Microsoft.AspNetCore.SignalR.Tests +{ + public class UncreatableHub: Hub + { + public UncreatableHub(object obj) + { + } + } +} diff --git a/test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs b/test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs index 65a82071a3..d77c8b70fd 100644 --- a/test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs +++ b/test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs @@ -290,29 +290,6 @@ namespace Microsoft.AspNetCore.Sockets.Tests Assert.False(exists); } - [Fact] - public async Task SynchronusExceptionEndsLongPollingConnection() - { - var manager = CreateConnectionManager(); - var state = manager.CreateConnection(); - - var dispatcher = new HttpConnectionDispatcher(manager, new LoggerFactory()); - var context = MakeRequest("/foo", state); - - var services = new ServiceCollection(); - services.AddEndPoint(); - var builder = new SocketBuilder(services.BuildServiceProvider()); - builder.UseEndPoint(); - var app = builder.Build(); - await dispatcher.ExecuteAsync(context, new HttpSocketOptions(), app); - - Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); - - ConnectionState removed; - bool exists = manager.TryGetConnection(state.Connection.ConnectionId, out removed); - Assert.False(exists); - } - [Fact] public async Task CompletedEndPointEndsLongPollingConnection() {