From ac261d878930870cabc9adde497c7213e4e257b9 Mon Sep 17 00:00:00 2001
From: Brennan <brecon@microsoft.com>
Date: Thu, 3 Oct 2019 08:27:41 -0700
Subject: [PATCH] Don't call close if connect does not succeed (#14114)

- small comment
- patch config
- fix assert
---
 eng/PatchConfig.props                         |  1 +
 .../ts/signalr/src/WebSocketTransport.ts      | 20 +++++++++++++-
 .../tests/ServerSentEventsTransport.test.ts   | 21 +++++++++++++++
 .../signalr/tests/WebSocketTransport.test.ts  | 27 +++++++++++++++++++
 4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props
index c139969aa4..1dfe2a7fc8 100644
--- a/eng/PatchConfig.props
+++ b/eng/PatchConfig.props
@@ -16,6 +16,7 @@ Directory.Build.props checks this property using the following condition:
   <PropertyGroup Condition=" '$(VersionPrefix)' == '3.0.1' ">
     <PackagesInPatch>
       Microsoft.AspNetCore.DataProtection.EntityFrameworkCore;
+      @microsoft/signalr;
     </PackagesInPatch>
   </PropertyGroup>
 </Project>
diff --git a/src/SignalR/clients/ts/signalr/src/WebSocketTransport.ts b/src/SignalR/clients/ts/signalr/src/WebSocketTransport.ts
index b7e842e3ec..418c306055 100644
--- a/src/SignalR/clients/ts/signalr/src/WebSocketTransport.ts
+++ b/src/SignalR/clients/ts/signalr/src/WebSocketTransport.ts
@@ -49,6 +49,7 @@ export class WebSocketTransport implements ITransport {
             url = url.replace(/^http/, "ws");
             let webSocket: WebSocket | undefined;
             const cookies = this.httpClient.getCookieString(url);
+            let opened = false;
 
             if (Platform.isNode && cookies) {
                 // Only pass cookies when in non-browser environments
@@ -72,6 +73,7 @@ export class WebSocketTransport implements ITransport {
             webSocket.onopen = (_event: Event) => {
                 this.logger.log(LogLevel.Information, `WebSocket connected to ${url}.`);
                 this.webSocket = webSocket;
+                opened = true;
                 resolve();
             };
 
@@ -94,7 +96,23 @@ export class WebSocketTransport implements ITransport {
                 }
             };
 
-            webSocket.onclose = (event: CloseEvent) => this.close(event);
+            webSocket.onclose = (event: CloseEvent) => {
+                // Don't call close handler if connection was never established
+                // We'll reject the connect call instead
+                if (opened) {
+                    this.close(event);
+                } else {
+                    let error: any = null;
+                    // ErrorEvent is a browser only type we need to check if the type exists before using it
+                    if (typeof ErrorEvent !== "undefined" && event instanceof ErrorEvent) {
+                        error = event.error;
+                    } else {
+                        error = new Error("There was an error with the transport.");
+                    }
+
+                    reject(error);
+                }
+            };
         });
     }
 
diff --git a/src/SignalR/clients/ts/signalr/tests/ServerSentEventsTransport.test.ts b/src/SignalR/clients/ts/signalr/tests/ServerSentEventsTransport.test.ts
index 0f017adace..bad6a9fb3a 100644
--- a/src/SignalR/clients/ts/signalr/tests/ServerSentEventsTransport.test.ts
+++ b/src/SignalR/clients/ts/signalr/tests/ServerSentEventsTransport.test.ts
@@ -45,6 +45,27 @@ describe("ServerSentEventsTransport", () => {
         });
     });
 
+    it("connect failure does not call onclose handler", async () => {
+        await VerifyLogger.run(async (logger) => {
+            const sse = new ServerSentEventsTransport(new TestHttpClient(), undefined, logger, true, TestEventSource);
+            let closeCalled = false;
+            sse.onclose = () => closeCalled = true;
+
+            const connectPromise = (async () => {
+                await sse.connect("http://example.com", TransferFormat.Text);
+            })();
+
+            await TestEventSource.eventSource.openSet;
+
+            TestEventSource.eventSource.onerror(new TestMessageEvent());
+
+            await expect(connectPromise)
+                .rejects
+                .toEqual(new Error("Error occurred"));
+            expect(closeCalled).toBe(false);
+        });
+    });
+
     [["http://example.com", "http://example.com?access_token=secretToken"],
     ["http://example.com?value=null", "http://example.com?value=null&access_token=secretToken"]]
         .forEach(([input, expected]) => {
diff --git a/src/SignalR/clients/ts/signalr/tests/WebSocketTransport.test.ts b/src/SignalR/clients/ts/signalr/tests/WebSocketTransport.test.ts
index 28277df11a..e26eaa86a3 100644
--- a/src/SignalR/clients/ts/signalr/tests/WebSocketTransport.test.ts
+++ b/src/SignalR/clients/ts/signalr/tests/WebSocketTransport.test.ts
@@ -66,6 +66,33 @@ describe("WebSocketTransport", () => {
         });
     });
 
+    it("connect failure does not call onclose handler", async () => {
+        await VerifyLogger.run(async (logger) => {
+            (global as any).ErrorEvent = TestErrorEvent;
+            const webSocket = new WebSocketTransport(new TestHttpClient(), undefined, logger, true, TestWebSocket);
+            let closeCalled = false;
+            webSocket.onclose = () => closeCalled = true;
+
+            let connectComplete: boolean = false;
+            const connectPromise = (async () => {
+                await webSocket.connect("http://example.com", TransferFormat.Text);
+                connectComplete = true;
+            })();
+
+            await TestWebSocket.webSocket.closeSet;
+
+            expect(connectComplete).toBe(false);
+
+            TestWebSocket.webSocket.onclose(new TestEvent());
+
+            await expect(connectPromise)
+                .rejects
+                .toThrow("There was an error with the transport.");
+            expect(connectComplete).toBe(false);
+            expect(closeCalled).toBe(false);
+        });
+    });
+
     [["http://example.com", "ws://example.com?access_token=secretToken"],
     ["http://example.com?value=null", "ws://example.com?value=null&access_token=secretToken"],
     ["https://example.com?value=null", "wss://example.com?value=null&access_token=secretToken"]]