From caff492cdc8eccc42254ec7d584ab48b1fb019fe Mon Sep 17 00:00:00 2001 From: Pawel Kadluczka Date: Mon, 23 Oct 2017 10:28:33 -0700 Subject: [PATCH] Removing sending OPTIONS request if WebSocket transport requested (#1036) Removing sending OPTIONS request if WebSocket transport requested This removes session stickiness requirement for WebSockets Fixes: #1035 --- .../Connection.spec.ts | 32 ++++++++++++ .../HttpConnection.ts | 27 ++++++---- .../DefaultTransportFactory.cs | 2 +- .../HttpConnection.cs | 44 ++++++++++------ .../DefaultTransportFactoryTests.cs | 17 ++++-- .../EndToEndTests.cs | 52 ++++++++++++++++++- 6 files changed, 144 insertions(+), 30 deletions(-) diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS.Tests/Connection.spec.ts b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS.Tests/Connection.spec.ts index 335dc9ffc1..42aa0ad162 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS.Tests/Connection.spec.ts +++ b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS.Tests/Connection.spec.ts @@ -192,6 +192,10 @@ describe("Connection", () => { }); eachTransport((requestedTransport: TransportType) => { + // OPTIONS is not sent when WebSockets transport is explicitly requested + if (requestedTransport === TransportType.WebSockets) { + return; + } it(`cannot be started if requested ${TransportType[requestedTransport]} transport not available on server`, async done => { let options: IHttpConnectionOptions = { httpClient: { @@ -244,6 +248,34 @@ describe("Connection", () => { } }); + it('does not send OPTIONS request if WebSockets transport requested explicitly', async done => { + let options: IHttpConnectionOptions = { + httpClient: { + options(url: string): Promise { + return Promise.reject("Should not be called"); + }, + get(url: string): Promise { + return Promise.reject("Should not be called"); + } + }, + transport: TransportType.WebSockets, + logging: null + } as IHttpConnectionOptions; + + let connection = new HttpConnection("http://tempuri.org", options); + try { + await connection.start(); + fail(); + done(); + } + catch (e) { + // WebSocket is created when the transport is connecting which happens after + // OPTIONS request would be sent. No better/easier way to test this. + expect(e.message).toBe("WebSocket is not defined"); + done(); + } + }); + [ [TransferMode.Text, TransferMode.Text], [TransferMode.Text, TransferMode.Binary], diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/HttpConnection.ts b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/HttpConnection.ts index 585815aba3..0967130f3f 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/HttpConnection.ts +++ b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/HttpConnection.ts @@ -55,18 +55,25 @@ export class HttpConnection implements IConnection { private async startInternal(): Promise { try { - let negotiatePayload = await this.httpClient.options(this.url); - let negotiateResponse: INegotiateResponse = JSON.parse(negotiatePayload); - this.connectionId = negotiateResponse.connectionId; + if (this.options.transport === TransportType.WebSockets) { + this.transport = this.createTransport(this.options.transport, [TransportType[TransportType.WebSockets]]); + } + else { + let negotiatePayload = await this.httpClient.options(this.url); + let negotiateResponse: INegotiateResponse = JSON.parse(negotiatePayload); + this.connectionId = negotiateResponse.connectionId; - // the user tries to stop the the connection when it is being started - if (this.connectionState == ConnectionState.Disconnected) { - return; + // 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.url.indexOf("?") == -1 ? "?" : "&") + `id=${this.connectionId}`; + this.transport = this.createTransport(this.options.transport, negotiateResponse.availableTransports); + } } - this.url += (this.url.indexOf("?") == -1 ? "?" : "&") + `id=${this.connectionId}`; - - this.transport = this.createTransport(this.options.transport, negotiateResponse.availableTransports); this.transport.onreceive = this.onreceive; this.transport.onclose = e => this.stopConnection(true, e); @@ -90,7 +97,7 @@ export class HttpConnection implements IConnection { } private createTransport(transport: TransportType | ITransport, availableTransports: string[]): ITransport { - if (!transport && availableTransports.length > 0) { + if ((transport === null || transport === undefined) && availableTransports.length > 0) { transport = TransportType[availableTransports[0]]; } if (transport === TransportType.WebSockets && availableTransports.indexOf(TransportType[transport]) >= 0) { diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/DefaultTransportFactory.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/DefaultTransportFactory.cs index baf02a5b67..155348e692 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client.Http/DefaultTransportFactory.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/DefaultTransportFactory.cs @@ -21,7 +21,7 @@ namespace Microsoft.AspNetCore.Sockets.Client throw new ArgumentOutOfRangeException(nameof(requestedTransportType)); } - if (httpClient == null) + if (httpClient == null && requestedTransportType != TransportType.WebSockets) { throw new ArgumentNullException(nameof(httpClient)); } diff --git a/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs b/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs index aa4fcf47fe..2bc0ac6c49 100644 --- a/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs +++ b/src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -42,6 +41,7 @@ namespace Microsoft.AspNetCore.Sockets.Client private ReadableChannel Input => _transportChannel.In; private WritableChannel Output => _transportChannel.Out; private readonly List _callbacks = new List(); + private readonly TransportType _requestedTransportType = TransportType.All; public Uri Url { get; } @@ -78,8 +78,14 @@ namespace Microsoft.AspNetCore.Sockets.Client _loggerFactory = loggerFactory ?? NullLoggerFactory.Instance; _logger = _loggerFactory.CreateLogger(); - _httpClient = httpMessageHandler == null ? new HttpClient() : new HttpClient(httpMessageHandler); - _httpClient.Timeout = HttpClientTimeout; + + _requestedTransportType = transportType; + if (_requestedTransportType != TransportType.WebSockets) + { + _httpClient = httpMessageHandler == null ? new HttpClient() : new HttpClient(httpMessageHandler); + _httpClient.Timeout = HttpClientTimeout; + } + _transportFactory = new DefaultTransportFactory(transportType, _loggerFactory, _httpClient); } @@ -130,19 +136,27 @@ namespace Microsoft.AspNetCore.Sockets.Client try { - var negotiationResponse = await Negotiate(Url, _httpClient, _logger); - _connectionId = negotiationResponse.ConnectionId; - - // Connection is being stopped while start was in progress - if (_connectionState == ConnectionState.Disconnected) + var connectUrl = Url; + if (_requestedTransportType == TransportType.WebSockets) { - _logger.HttpConnectionClosed(_connectionId); - return; + _transport = _transportFactory.CreateTransport(TransportType.WebSockets); + } + else + { + var negotiationResponse = await Negotiate(Url, _httpClient, _logger); + _connectionId = negotiationResponse.ConnectionId; + + // Connection is being stopped while start was in progress + if (_connectionState == ConnectionState.Disconnected) + { + _logger.HttpConnectionClosed(_connectionId); + return; + } + + _transport = _transportFactory.CreateTransport(GetAvailableServerTransports(negotiationResponse)); + connectUrl = CreateConnectUrl(Url, negotiationResponse); } - _transport = _transportFactory.CreateTransport(GetAvailableServerTransports(negotiationResponse)); - - var connectUrl = CreateConnectUrl(Url, negotiationResponse); _logger.StartingTransport(_connectionId, _transport.GetType().Name, connectUrl); await StartTransport(connectUrl); } @@ -174,7 +188,7 @@ namespace Microsoft.AspNetCore.Sockets.Client await _eventQueue.Drain(); await Task.WhenAny(_eventQueue.Drain().NoThrow(), Task.Delay(_eventQueueDrainTimeout)); - _httpClient.Dispose(); + _httpClient?.Dispose(); _logger.RaiseClosed(_connectionId); @@ -449,7 +463,7 @@ namespace Microsoft.AspNetCore.Sockets.Client await _receiveLoopTask; } - _httpClient.Dispose(); + _httpClient?.Dispose(); } public IDisposable OnReceived(Func callback, object state) diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/DefaultTransportFactoryTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/DefaultTransportFactoryTests.cs index 601f10ac21..a8a356abbd 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/DefaultTransportFactoryTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/DefaultTransportFactoryTests.cs @@ -23,15 +23,26 @@ namespace Microsoft.AspNetCore.SignalR.Tests () => new DefaultTransportFactory(transportType, new LoggerFactory(), new HttpClient())); } - [Fact] - public void DefaultTransportFactoryCannotBeCreatedWithoutHttpClient() + [Theory] + [InlineData(TransportType.All)] + [InlineData(TransportType.LongPolling)] + [InlineData(TransportType.ServerSentEvents)] + [InlineData(TransportType.LongPolling | TransportType.WebSockets)] + [InlineData(TransportType.ServerSentEvents | TransportType.WebSockets)] + public void DefaultTransportFactoryCannotBeCreatedWithoutHttpClient(TransportType transportType) { var exception = Assert.Throws( - () => new DefaultTransportFactory(TransportType.All, new LoggerFactory(), httpClient: null)); + () => new DefaultTransportFactory(transportType, new LoggerFactory(), httpClient: null)); Assert.Equal("httpClient", exception.ParamName); } + [Fact] + public void DefaultTransportFactoryCanBeCreatedWithoutHttpClientIfWebSocketsTransportRequestedExplicitly() + { + new DefaultTransportFactory(TransportType.WebSockets, new LoggerFactory(), httpClient: null); + } + [ConditionalTheory] [InlineData(TransportType.WebSockets, typeof(WebSocketsTransport))] [InlineData(TransportType.ServerSentEvents, typeof(ServerSentEventsTransport))] diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs index 709a7156a8..326e15e855 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/EndToEndTests.cs @@ -11,11 +11,13 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client; using Microsoft.AspNetCore.SignalR.Tests.Common; using Microsoft.AspNetCore.Sockets; -using Microsoft.AspNetCore.Sockets.Features; using Microsoft.AspNetCore.Sockets.Client; +using Microsoft.AspNetCore.Sockets.Features; using Microsoft.AspNetCore.Testing.xunit; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Testing; +using Moq; +using Moq.Protected; using Xunit; using Xunit.Abstractions; @@ -97,6 +99,54 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [ConditionalFact] + [OSSkipCondition(OperatingSystems.Windows, WindowsVersions.Win7, WindowsVersions.Win2008R2, SkipReason = "No WebSockets Client for this platform")] + public async Task HTTPRequestsNotSentWhenWebSocketsTransportRequested() + { + using (StartLog(out var loggerFactory, testName: nameof(HTTPRequestsNotSentWhenWebSocketsTransportRequested))) + { + var logger = loggerFactory.CreateLogger(); + var url = _serverFixture.BaseUrl + "/echo"; + + var mockHttpHandler = new Mock(); + mockHttpHandler.Protected() + .Setup>("SendAsync", ItExpr.IsAny(), ItExpr.IsAny()) + .Returns( + (request, cancellationToken) => Task.FromException(new InvalidOperationException("HTTP requests should not be sent."))); + + var connection = new HttpConnection(new Uri(url), TransportType.WebSockets, loggerFactory, mockHttpHandler.Object); + + try + { + var receiveTcs = new TaskCompletionSource(); + connection.OnReceived((data, state) => + { + var tcs = (TaskCompletionSource)state; + tcs.TrySetResult(data); + return Task.CompletedTask; + }, receiveTcs); + + var message = new byte[] { 42 }; + await connection.StartAsync().OrTimeout(); + await connection.SendAsync(message).OrTimeout(); + + var receivedData = await receiveTcs.Task.OrTimeout(); + Assert.Equal(message, receivedData); + } + catch (Exception ex) + { + logger.LogInformation(ex, "Test threw exception"); + throw; + } + finally + { + logger.LogInformation("Disposing Connection"); + await connection.DisposeAsync().OrTimeout(); + logger.LogInformation("Disposed Connection"); + } + } + } + [Theory] [MemberData(nameof(TransportTypesAndTransferModes))] public async Task ConnectionCanSendAndReceiveMessages(TransportType transportType, TransferMode requestedTransferMode)