From 841ceb24b6fca2644002f6e9c6330bebd6007fde Mon Sep 17 00:00:00 2001 From: moozzyk Date: Wed, 29 Mar 2017 17:24:51 -0700 Subject: [PATCH] Fixing start/stop race in the TS client --- .../Connection.spec.ts | 29 +++++++++ .../Connection.ts | 65 ++++++++++++++----- .../wwwroot/js/hubConnectionTests.js | 10 ++- samples/SocketsSample/wwwroot/hubs.html | 12 ++-- 4 files changed, 91 insertions(+), 25 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 1275a92cb9..ba1ae6bb90 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 @@ -3,6 +3,7 @@ import { Connection } from "../Microsoft.AspNetCore.SignalR.Client.TS/Connection import { ISignalROptions } from "../Microsoft.AspNetCore.SignalR.Client.TS/ISignalROptions" describe("Connection", () => { + it("starting connection fails if getting id fails", async (done) => { let options: ISignalROptions = { httpClient: { @@ -93,4 +94,32 @@ describe("Connection", () => { done(); } }); + + it("can stop a starting connection", async (done) => { + let options: ISignalROptions = { + httpClient: { + get(url: string): Promise { + connection.stop(); + return Promise.resolve(""); + } + } + } as ISignalROptions; + + var connection = new Connection("http://tempuri.org", undefined, options); + + try { + await connection.start(); + done(); + } + catch (e) { + fail(); + done(); + } + }); + + it("can stop a non-started connection", async (done) => { + var connection = new Connection("http://tempuri.org"); + await connection.stop(); + done(); + }); }); diff --git a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Connection.ts b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Connection.ts index 74944fc46d..69de36fffb 100644 --- a/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Connection.ts +++ b/client-ts/Microsoft.AspNetCore.SignalR.Client.TS/Connection.ts @@ -18,6 +18,7 @@ export class Connection implements IConnection { private connectionId: string; private httpClient: IHttpClient; private transport: ITransport; + private startPromise: Promise; constructor(url: string, queryString: string = "", options: ISignalROptions = {}) { this.url = url; @@ -28,20 +29,33 @@ export class Connection implements IConnection { async start(transportType: TransportType = TransportType.WebSockets): Promise { if (this.connectionState != ConnectionState.Initial) { - throw new Error("Cannot start a connection that is not in the 'Initial' state."); + return Promise.reject(new Error("Cannot start a connection that is not in the 'Initial' state.")); } this.connectionState = ConnectionState.Connecting; - this.transport = this.createTransport(transportType); - this.transport.onDataReceived = this.onDataReceived; - this.transport.onClosed = e => this.stopConnection(e); + this.startPromise = this.startInternal(transportType); + return this.startPromise; + } + private async startInternal(transportType: TransportType): Promise { try { this.connectionId = await this.httpClient.get(`${this.url}/negotiate?${this.queryString}`); + + // the user tries to stop the the connection when it is being started + if (this.connectionState == ConnectionState.Disconnected) { + return; + } + this.queryString = `id=${this.connectionId}`; + + this.transport = this.createTransport(transportType); + this.transport.onDataReceived = this.onDataReceived; + this.transport.onClosed = e => this.stopConnection(true, e); await this.transport.connect(this.url, this.queryString); - this.connectionState = ConnectionState.Connected; + // 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); } catch(e) { console.log("Failed to start the connection. " + e) @@ -65,27 +79,44 @@ export class Connection implements IConnection { throw new Error("No valid transports requested."); } + private changeState(from: ConnectionState, to: ConnectionState): Boolean { + if (this.connectionState == from) { + this.connectionState = to; + return true; + } + return false; + } + send(data: any): Promise { if (this.connectionState != ConnectionState.Connected) { throw new Error("Cannot send data if the connection is not in the 'Connected' State"); } + return this.transport.send(data); } - stop(): void { - if (this.connectionState != ConnectionState.Connected) { - throw new Error("Cannot stop the connection if it is not in the 'Connected' State"); - } - - this.stopConnection(); - } - - private stopConnection(error?: any) { - this.transport.stop(); - this.transport = null; + async stop(): Promise { + let previousState = this.connectionState; this.connectionState = ConnectionState.Disconnected; - if (this.onClosed) { + try { + await this.startPromise; + } + catch (e) { + // this exception is returned to the user as a rejected Promise from the start method + } + this.stopConnection(/*raiseClosed*/ previousState == ConnectionState.Connected); + } + + private stopConnection(raiseClosed: Boolean, error?: any) { + if (this.transport) { + this.transport.stop(); + this.transport = null; + } + + this.connectionState = ConnectionState.Disconnected; + + if (raiseClosed && this.onClosed) { this.onClosed(error); } } 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 99d172bac6..abc326f7dd 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 @@ -44,9 +44,11 @@ describe('hubConnection', () => { expect(e.message).toBe(errorMessage); }) .then(() => { - hubConnection.stop(); - done(); + return hubConnection.stop(); }) + .then(() => { + done(); + }); }) .catch(() => { fail(); @@ -70,7 +72,9 @@ describe('hubConnection', () => { return Promise.all([client.invoke('InvokeWithString', message), callbackPromise]); }) .then(() => { - stop(); + return stop(); + }) + .then(() => { done(); }) .catch(e => { diff --git a/samples/SocketsSample/wwwroot/hubs.html b/samples/SocketsSample/wwwroot/hubs.html index 027665a34c..cb2331db55 100644 --- a/samples/SocketsSample/wwwroot/hubs.html +++ b/samples/SocketsSample/wwwroot/hubs.html @@ -117,14 +117,16 @@ click('connect', event => { isConnected = true; addLine('Connected successfully', 'green'); }) - .catch(err => { - addLine(err, 'red'); - }); + .catch(err => { + addLine(err, 'red'); + }); }); click('disconnect', event => { - connection.stop(); - isConnected = false; + connection.stop() + .then(() => { + isConnected = false; + }); }); click('broadcast', event => {