From 614908248d20c4a1572d1a3bbc7a997d12be9d01 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Mon, 15 Mar 2021 13:53:49 -0700 Subject: [PATCH 1/4] [SignalR TS] Fix permanent Disconnecting state --- .../clients/ts/signalr/src/HttpConnection.ts | 2 + .../clients/ts/signalr/src/HubConnection.ts | 1 + .../tests/HubConnection.Reconnect.test.ts | 93 +++++++++++++++++++ .../clients/ts/signalr/tests/TestWebSocket.ts | 54 +++++++++++ 4 files changed, 150 insertions(+) diff --git a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts index 1f1bb2451bb1..490db7970869 100644 --- a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts @@ -470,6 +470,8 @@ export class HttpConnection implements IConnection { if (this._connectionState === ConnectionState.Disconnected) { this._logger.log(LogLevel.Debug, `Call to HttpConnection.stopConnection(${error}) was ignored because the connection is already in the disconnected state.`); + // Make sure HttpConnection.stop() won't be blocked on the promise + this._stopPromiseResolver(); return; } diff --git a/src/SignalR/clients/ts/signalr/src/HubConnection.ts b/src/SignalR/clients/ts/signalr/src/HubConnection.ts index 1c1a6c484e2c..e2329c28d9a9 100644 --- a/src/SignalR/clients/ts/signalr/src/HubConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HubConnection.ts @@ -769,6 +769,7 @@ export class HubConnection { if (this._connectionState !== HubConnectionState.Reconnecting) { this._logger.log(LogLevel.Debug, "Connection left the reconnecting state during reconnect attempt. Done reconnecting."); + this._completeClose(); return; } diff --git a/src/SignalR/clients/ts/signalr/tests/HubConnection.Reconnect.test.ts b/src/SignalR/clients/ts/signalr/tests/HubConnection.Reconnect.test.ts index 15e5e953a0d7..756734db2fc5 100644 --- a/src/SignalR/clients/ts/signalr/tests/HubConnection.Reconnect.test.ts +++ b/src/SignalR/clients/ts/signalr/tests/HubConnection.Reconnect.test.ts @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. import { DefaultReconnectPolicy } from "../src/DefaultReconnectPolicy"; +import { HttpConnection, INegotiateResponse } from "../src/HttpConnection"; import { HubConnection, HubConnectionState } from "../src/HubConnection"; +import { IHttpConnectionOptions } from "../src/IHttpConnectionOptions"; import { MessageType } from "../src/IHubProtocol"; import { RetryContext } from "../src/IRetryPolicy"; import { JsonHubProtocol } from "../src/JsonHubProtocol"; @@ -10,6 +12,8 @@ import { JsonHubProtocol } from "../src/JsonHubProtocol"; import { VerifyLogger } from "./Common"; import { TestConnection } from "./TestConnection"; import { PromiseSource } from "./Utils"; +import { TestHttpClient } from "./TestHttpClient"; +import { TestWebSocket, TestEvent, TestMessageEvent } from "./TestWebSocket"; describe("auto reconnect", () => { it("is not enabled by default", async () => { @@ -785,4 +789,93 @@ describe("auto reconnect", () => { } }); }); + + it("can be stopped while restarting the underlying connection and negotiate throws", async () => { + await VerifyLogger.run(async (logger) => { + let onreconnectingCount = 0; + let onreconnectedCount = 0; + let closeCount = 0; + + const nextRetryDelayCalledPromise = new PromiseSource(); + + const defaultConnectionId = "abc123"; + const defaultConnectionToken = "123abc"; + const defaultNegotiateResponse: INegotiateResponse = { + availableTransports: [ + { transport: "WebSockets", transferFormats: ["Text", "Binary"] }, + { transport: "ServerSentEvents", transferFormats: ["Text"] }, + { transport: "LongPolling", transferFormats: ["Text", "Binary"] }, + ], + connectionId: defaultConnectionId, + connectionToken: defaultConnectionToken, + negotiateVersion: 1, + }; + + const startStarted = new PromiseSource(); + let negotiateCount = 0; + + const options: IHttpConnectionOptions = { + WebSocket: TestWebSocket, + httpClient: new TestHttpClient() + .on("POST", async () => { + ++negotiateCount; + if (negotiateCount === 1) { + return defaultNegotiateResponse; + } + startStarted.resolve(); + return Promise.reject("Error with negotiate"); + }) + .on("GET", () => ""), + logger, + } as IHttpConnectionOptions; + + const connection = new HttpConnection("http://tempuri.org", options); + const hubConnection = HubConnection.create(connection, logger, new JsonHubProtocol(), { + nextRetryDelayInMilliseconds() { + nextRetryDelayCalledPromise.resolve(); + return 0; + }, + }); + + hubConnection.onreconnecting(() => { + onreconnectingCount++; + }); + + hubConnection.onreconnected(() => { + onreconnectedCount++; + }); + + hubConnection.onclose(() => { + closeCount++; + }); + + TestWebSocket.webSocketSet = new PromiseSource(); + const startPromise = hubConnection.start(); + await TestWebSocket.webSocketSet; + await TestWebSocket.webSocket.openSet; + TestWebSocket.webSocket.onopen(new TestEvent()); + TestWebSocket.webSocket.onmessage(new TestMessageEvent("{}\x1e")); + + await startPromise; + TestWebSocket.webSocket.close(); + TestWebSocket.webSocketSet = new PromiseSource(); + + await nextRetryDelayCalledPromise; + + expect(hubConnection.state).toBe(HubConnectionState.Reconnecting); + expect(onreconnectingCount).toBe(1); + expect(onreconnectedCount).toBe(0); + expect(closeCount).toBe(0); + + await startStarted; + await hubConnection.stop(); + + expect(hubConnection.state).toBe(HubConnectionState.Disconnected); + expect(onreconnectingCount).toBe(1); + expect(onreconnectedCount).toBe(0); + expect(closeCount).toBe(1); + }, + "Failed to complete negotiation with the server: Error with negotiate", + "Failed to start the connection: Error with negotiate"); + }); }); diff --git a/src/SignalR/clients/ts/signalr/tests/TestWebSocket.ts b/src/SignalR/clients/ts/signalr/tests/TestWebSocket.ts index a467c17798b5..dc1e1a01aa33 100644 --- a/src/SignalR/clients/ts/signalr/tests/TestWebSocket.ts +++ b/src/SignalR/clients/ts/signalr/tests/TestWebSocket.ts @@ -231,3 +231,57 @@ export class TestCloseEvent implements Event { public CAPTURING_PHASE: number = 0; public NONE: number = 0; } + +export class TestMessageEvent implements MessageEvent { + constructor(data: any) { + this.data = data; + } + public data: any; + public lastEventId: string = ""; + public origin: string = ""; + public ports: readonly MessagePort[] = []; + public source: MessagePort | Window | ServiceWorker | null = null; + public composed: boolean = false; + public composedPath(): EventTarget[]; + public composedPath(): any[] { + throw new Error("Method not implemented."); + } + public code: number = 0; + public reason: string = ""; + public wasClean: boolean = false; + public initCloseEvent(typeArg: string, canBubbleArg: boolean, cancelableArg: boolean, wasCleanArg: boolean, codeArg: number, reasonArg: string): void { + throw new Error("Method not implemented."); + } + public bubbles: boolean = false; + public cancelBubble: boolean = false; + public cancelable: boolean = false; + public currentTarget!: EventTarget; + public defaultPrevented: boolean = false; + public eventPhase: number = 0; + public isTrusted: boolean = false; + public returnValue: boolean = false; + public scoped: boolean = false; + public srcElement!: Element | null; + public target!: EventTarget; + public timeStamp: number = 0; + public type: string = ""; + public deepPath(): EventTarget[] { + throw new Error("Method not implemented."); + } + public initEvent(type: string, bubbles?: boolean | undefined, cancelable?: boolean | undefined): void { + throw new Error("Method not implemented."); + } + public preventDefault(): void { + throw new Error("Method not implemented."); + } + public stopImmediatePropagation(): void { + throw new Error("Method not implemented."); + } + public stopPropagation(): void { + throw new Error("Method not implemented."); + } + public AT_TARGET: number = 0; + public BUBBLING_PHASE: number = 0; + public CAPTURING_PHASE: number = 0; + public NONE: number = 0; +} From be3ebae75470d42e6d8d3301bbc40e678eef9604 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Fri, 19 Mar 2021 10:41:36 -0700 Subject: [PATCH 2/4] fb --- .../clients/ts/signalr/src/HttpConnection.ts | 4 +++- .../clients/ts/signalr/src/HubConnection.ts | 7 +++++-- .../ts/signalr/tests/HttpConnection.test.ts | 19 +++++++++++++------ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts index 490db7970869..58086c2f1cc9 100644 --- a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts @@ -215,7 +215,6 @@ export class HttpConnection implements IConnection { this.transport = undefined; } else { this._logger.log(LogLevel.Debug, "HttpConnection.transport is undefined in HttpConnection.stop() because start() failed."); - this._stopConnection(); } } @@ -295,6 +294,9 @@ export class HttpConnection implements IConnection { this._logger.log(LogLevel.Error, "Failed to start the connection: " + e); this._connectionState = ConnectionState.Disconnected; this.transport = undefined; + if (this._stopPromiseResolver !== undefined) { + this._stopPromiseResolver(); + } return Promise.reject(e); } } diff --git a/src/SignalR/clients/ts/signalr/src/HubConnection.ts b/src/SignalR/clients/ts/signalr/src/HubConnection.ts index e2329c28d9a9..2e68ff465359 100644 --- a/src/SignalR/clients/ts/signalr/src/HubConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HubConnection.ts @@ -768,8 +768,11 @@ export class HubConnection { this._logger.log(LogLevel.Information, `Reconnect attempt failed because of error '${e}'.`); if (this._connectionState !== HubConnectionState.Reconnecting) { - this._logger.log(LogLevel.Debug, "Connection left the reconnecting state during reconnect attempt. Done reconnecting."); - this._completeClose(); + this._logger.log(LogLevel.Debug, `Connection moved to the '${this._connectionState}' from the reconnecting state during reconnect attempt. Done reconnecting.`); + // The TypeScript compiler thinks that connectionState must be Connected here. The TypeScript compiler is wrong. + if (this._connectionState as any === HubConnectionState.Disconnecting) { + this._completeClose(); + } return; } diff --git a/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts b/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts index d2265ccf50ce..f25d4ba4ab73 100644 --- a/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts +++ b/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts @@ -137,23 +137,30 @@ describe("HttpConnection", () => { it("can stop a starting connection", async () => { await VerifyLogger.run(async (logger) => { + const stoppingPromise = new PromiseSource(); + const startingPromise = new PromiseSource(); const options: IHttpConnectionOptions = { ...commonOptions, httpClient: new TestHttpClient() .on("POST", async () => { - await connection.stop(); + startingPromise.resolve(); + await stoppingPromise; return "{}"; - }) - .on("GET", async () => { - await connection.stop(); - return ""; }), logger, } as IHttpConnectionOptions; const connection = new HttpConnection("http://tempuri.org", options); - await expect(connection.start(TransferFormat.Text)) + const startPromise = connection.start(TransferFormat.Text) + + await startingPromise; + const stopPromise = connection.stop(); + stoppingPromise.resolve(); + + await stopPromise; + + await expect(startPromise) .rejects .toThrow("The connection was stopped during negotiation."); }, From 8802cad07a5ab058995d6890b8596878c2484772 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Fri, 19 Mar 2021 10:45:23 -0700 Subject: [PATCH 3/4] comment --- src/SignalR/clients/ts/signalr/src/HttpConnection.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts index 58086c2f1cc9..01716f638f9f 100644 --- a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts @@ -52,7 +52,7 @@ export class HttpConnection implements IConnection { private transport?: ITransport; private _startInternalPromise?: Promise; private _stopPromise?: Promise; - private _stopPromiseResolver!: (value?: PromiseLike) => void; + private _stopPromiseResolver: (value?: PromiseLike) => void = () => {}; private _stopError?: Error; private _accessTokenFactory?: () => string | Promise; private _sendQueue?: TransportSendQueue; @@ -294,9 +294,9 @@ export class HttpConnection implements IConnection { this._logger.log(LogLevel.Error, "Failed to start the connection: " + e); this._connectionState = ConnectionState.Disconnected; this.transport = undefined; - if (this._stopPromiseResolver !== undefined) { - this._stopPromiseResolver(); - } + + // if start fails, any active calls to stop assume that start will complete the stop promise + this._stopPromiseResolver(); return Promise.reject(e); } } From c6a1d18e12f25ad2e9464558ec532270249d9372 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Fri, 19 Mar 2021 10:47:52 -0700 Subject: [PATCH 4/4] whoops --- src/SignalR/clients/ts/signalr/src/HttpConnection.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts index 01716f638f9f..dccaf4c57ed6 100644 --- a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts @@ -472,8 +472,6 @@ export class HttpConnection implements IConnection { if (this._connectionState === ConnectionState.Disconnected) { this._logger.log(LogLevel.Debug, `Call to HttpConnection.stopConnection(${error}) was ignored because the connection is already in the disconnected state.`); - // Make sure HttpConnection.stop() won't be blocked on the promise - this._stopPromiseResolver(); return; }