[TS] Remove extra negotiates from common scenarios (#11191)

src/SignalR/clients/ts/signalr/src/HttpConnection.ts

@@ -352,26 +352,33 @@ export class HttpConnection implements IConnection {
         const transportExceptions: any[] = [];
         const transports = negotiateResponse.availableTransports || [];
         for (const endpoint of transports) {
-            try {
+            const transportOrError = this.resolveTransportOrError(endpoint, requestedTransport, requestedTransferFormat);
-                const transport = this.resolveTransport(endpoint, requestedTransport, requestedTransferFormat);
+            if (transportOrError instanceof Error) {
-                if (typeof transport === "number") {
+                // Store the error and continue, we don't want to cause a re-negotiate in these cases
-                    this.transport = this.constructTransport(transport);
+                transportExceptions.push(`${endpoint.transport} failed: ${transportOrError}`);
-                    if (!negotiateResponse.connectionId) {
+            } else if (this.isITransport(transportOrError)) {
+                this.transport = transportOrError;
+                if (!negotiateResponse.connectionId) {
+                    try {
                         negotiateResponse = await this.getNegotiationResponse(url);
-                        connectUrl = this.createConnectUrl(url, negotiateResponse.connectionId);
+                    } catch (ex) {
+                        return Promise.reject(ex);
                     }
+                    connectUrl = this.createConnectUrl(url, negotiateResponse.connectionId);
+                }
+                try {
                     await this.transport!.connect(connectUrl, requestedTransferFormat);
                     return;
-                }
+                } catch (ex) {
-            } catch (ex) {
+                    this.logger.log(LogLevel.Error, `Failed to start the transport '${endpoint.transport}': ${ex}`);
-                this.logger.log(LogLevel.Error, `Failed to start the transport '${endpoint.transport}': ${ex}`);
+                    negotiateResponse.connectionId = undefined;
-                negotiateResponse.connectionId = undefined;
+                    transportExceptions.push(`${endpoint.transport} failed: ${ex}`);
-                transportExceptions.push(`${endpoint.transport} failed: ${ex}`);
 
-                if (this.connectionState !== ConnectionState.Connecting) {
+                    if (this.connectionState !== ConnectionState.Connecting) {
-                    const message = "Failed to select transport before stop() was called.";
+                        const message = "Failed to select transport before stop() was called.";
-                    this.logger.log(LogLevel.Debug, message);
+                        this.logger.log(LogLevel.Debug, message);
-                    return Promise.reject(new Error(message));
+                        return Promise.reject(new Error(message));
+                    }
                 }
             }
         }
@@ -382,7 +389,7 @@ export class HttpConnection implements IConnection {
         return Promise.reject(new Error("None of the transports supported by the client are supported by the server."));
     }
 
-    private constructTransport(transport: HttpTransportType) {
+    private constructTransport(transport: HttpTransportType): ITransport {
         switch (transport) {
             case HttpTransportType.WebSockets:
                 if (!this.options.WebSocket) {
@@ -401,32 +408,36 @@ export class HttpConnection implements IConnection {
         }
     }
 
-    private resolveTransport(endpoint: IAvailableTransport, requestedTransport: HttpTransportType | undefined, requestedTransferFormat: TransferFormat): HttpTransportType | null {
+    private resolveTransportOrError(endpoint: IAvailableTransport, requestedTransport: HttpTransportType | undefined, requestedTransferFormat: TransferFormat): ITransport | Error {
         const transport = HttpTransportType[endpoint.transport];
         if (transport === null || transport === undefined) {
             this.logger.log(LogLevel.Debug, `Skipping transport '${endpoint.transport}' because it is not supported by this client.`);
+            return new Error(`Skipping transport '${endpoint.transport}' because it is not supported by this client.`);
         } else {
-            const transferFormats = endpoint.transferFormats.map((s) => TransferFormat[s]);
             if (transportMatches(requestedTransport, transport)) {
+                const transferFormats = endpoint.transferFormats.map((s) => TransferFormat[s]);
                 if (transferFormats.indexOf(requestedTransferFormat) >= 0) {
                     if ((transport === HttpTransportType.WebSockets && !this.options.WebSocket) ||
                         (transport === HttpTransportType.ServerSentEvents && !this.options.EventSource)) {
                         this.logger.log(LogLevel.Debug, `Skipping transport '${HttpTransportType[transport]}' because it is not supported in your environment.'`);
-                        throw new Error(`'${HttpTransportType[transport]}' is not supported in your environment.`);
+                        return new Error(`'${HttpTransportType[transport]}' is not supported in your environment.`);
                     } else {
                         this.logger.log(LogLevel.Debug, `Selecting transport '${HttpTransportType[transport]}'.`);
-                        return transport;
+                        try {
+                            return this.constructTransport(transport);
+                        } catch (ex) {
+                            return ex;
+                        }
                     }
                 } else {
                     this.logger.log(LogLevel.Debug, `Skipping transport '${HttpTransportType[transport]}' because it does not support the requested transfer format '${TransferFormat[requestedTransferFormat]}'.`);
-                    throw new Error(`'${HttpTransportType[transport]}' does not support ${TransferFormat[requestedTransferFormat]}.`);
+                    return new Error(`'${HttpTransportType[transport]}' does not support ${TransferFormat[requestedTransferFormat]}.`);
                 }
             } else {
                 this.logger.log(LogLevel.Debug, `Skipping transport '${HttpTransportType[transport]}' because it was disabled by the client.`);
-                throw new Error(`'${HttpTransportType[transport]}' is disabled by the client.`);
+                return new Error(`'${HttpTransportType[transport]}' is disabled by the client.`);
             }
         }
-        return null;
     }
 
     private isITransport(transport: any): transport is ITransport {

src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts

@@ -192,13 +192,17 @@ describe("HttpConnection", () => {
         "Failed to start the connection: Error: Unexpected status code returned from negotiate 999");
     });
 
-    it("all transport failure error get aggregated", async () => {
+    it("all transport failure errors get aggregated", async () => {
         await VerifyLogger.run(async (loggerImpl) => {
+            let negotiateCount: number = 0;
             const options: IHttpConnectionOptions = {
                 WebSocket: false,
                 ...commonOptions,
                 httpClient: new TestHttpClient()
-                    .on("POST", () => defaultNegotiateResponse)
+                    .on("POST", () =>  {
+                        negotiateCount++;
+                        return defaultNegotiateResponse;
+                    })
                     .on("GET", () => new HttpResponse(200))
                     .on("DELETE", () => new HttpResponse(202)),
 
@@ -210,13 +214,77 @@ describe("HttpConnection", () => {
             await expect(connection.start(TransferFormat.Text))
                 .rejects
                 .toThrow("Unable to connect to the server with any of the available transports. WebSockets failed: null ServerSentEvents failed: Error: 'ServerSentEvents' is disabled by the client. LongPolling failed: Error: 'LongPolling' is disabled by the client.");
+
+            expect(negotiateCount).toEqual(1);
         },
         "Failed to start the transport 'WebSockets': null",
-        "Failed to start the transport 'ServerSentEvents': Error: 'ServerSentEvents' is disabled by the client.",
-        "Failed to start the transport 'LongPolling': Error: 'LongPolling' is disabled by the client.",
         "Failed to start the connection: Error: Unable to connect to the server with any of the available transports. WebSockets failed: null ServerSentEvents failed: Error: 'ServerSentEvents' is disabled by the client. LongPolling failed: Error: 'LongPolling' is disabled by the client.");
     });
 
+    it("negotiate called again when transport fails to start and falls back", async () => {
+        await VerifyLogger.run(async (loggerImpl) => {
+            let negotiateCount: number = 0;
+            const options: IHttpConnectionOptions = {
+                EventSource: () => { throw new Error("Don't allow ServerSentEvents."); },
+                WebSocket: () => { throw new Error("Don't allow Websockets."); },
+                ...commonOptions,
+                httpClient: new TestHttpClient()
+                    .on("POST", () =>  {
+                        negotiateCount++;
+                        return defaultNegotiateResponse;
+                    })
+                    .on("GET", () => new HttpResponse(200))
+                    .on("DELETE", () => new HttpResponse(202)),
+
+                logger: loggerImpl,
+                transport: HttpTransportType.WebSockets | HttpTransportType.ServerSentEvents,
+            } as IHttpConnectionOptions;
+
+            const connection = new HttpConnection("http://tempuri.org", options);
+            await expect(connection.start(TransferFormat.Text))
+                .rejects
+                .toThrow("Unable to connect to the server with any of the available transports. WebSockets failed: Error: Don't allow Websockets. ServerSentEvents failed: Error: Don't allow ServerSentEvents. LongPolling failed: Error: 'LongPolling' is disabled by the client.");
+
+            expect(negotiateCount).toEqual(2);
+        },
+        "Failed to start the transport 'WebSockets': Error: Don't allow Websockets.",
+        "Failed to start the transport 'ServerSentEvents': Error: Don't allow ServerSentEvents.",
+        "Failed to start the connection: Error: Unable to connect to the server with any of the available transports. WebSockets failed: Error: Don't allow Websockets. ServerSentEvents failed: Error: Don't allow ServerSentEvents. LongPolling failed: Error: 'LongPolling' is disabled by the client.");
+    });
+
+    it("failed re-negotiate fails start", async () => {
+        await VerifyLogger.run(async (loggerImpl) => {
+            let negotiateCount: number = 0;
+            const options: IHttpConnectionOptions = {
+                EventSource: () => { throw new Error("Don't allow ServerSentEvents."); },
+                WebSocket: () => { throw new Error("Don't allow Websockets."); },
+                ...commonOptions,
+                httpClient: new TestHttpClient()
+                    .on("POST", () =>  {
+                        negotiateCount++;
+                        if (negotiateCount === 2) {
+                            throw new Error("negotiate failed");
+                        }
+                        return defaultNegotiateResponse;
+                    })
+                    .on("GET", () => new HttpResponse(200))
+                    .on("DELETE", () => new HttpResponse(202)),
+
+                logger: loggerImpl,
+            } as IHttpConnectionOptions;
+
+            const connection = new HttpConnection("http://tempuri.org", options);
+            await expect(connection.start(TransferFormat.Text))
+                .rejects
+                .toThrow("negotiate failed");
+
+            expect(negotiateCount).toEqual(2);
+        },
+        "Failed to start the transport 'WebSockets': Error: Don't allow Websockets.",
+        "Failed to complete negotiation with the server: Error: negotiate failed",
+        "Failed to start the connection: Error: negotiate failed");
+    });
+
     it("can stop a non-started connection", async () => {
         await VerifyLogger.run(async (logger) => {
             const connection = new HttpConnection("http://tempuri.org", { ...commonOptions, logger });
@@ -344,8 +412,7 @@ describe("HttpConnection", () => {
                     .toThrow(`Unable to connect to the server with any of the available transports. ${negotiateResponse.availableTransports[0].transport} failed: Error: '${negotiateResponse.availableTransports[0].transport}' is disabled by the client.` +
                     ` ${negotiateResponse.availableTransports[1].transport} failed: Error: '${negotiateResponse.availableTransports[1].transport}' is disabled by the client.`);
             },
-            /Failed to start the connection: Error: Unable to connect to the server with any of the available transports. [a-zA-Z]+\b failed: Error: '[a-zA-Z]+\b' is disabled by the client. [a-zA-Z]+\b failed: Error: '[a-zA-Z]+\b' is disabled by the client./,
+            /Failed to start the connection: Error: Unable to connect to the server with any of the available transports. [a-zA-Z]+\b failed: Error: '[a-zA-Z]+\b' is disabled by the client. [a-zA-Z]+\b failed: Error: '[a-zA-Z]+\b' is disabled by the client./);
-            /Failed to start the transport '[a-zA-Z]+': Error: '[a-zA-Z]+' is disabled by the client./);
         });
 
         it(`cannot be started if server's only transport (${HttpTransportType[requestedTransport]}) is masked out by the transport option`, async () => {
@@ -384,8 +451,7 @@ describe("HttpConnection", () => {
                     expect(e.message).toBe(`Unable to connect to the server with any of the available transports. ${HttpTransportType[requestedTransport]} failed: Error: '${HttpTransportType[requestedTransport]}' is disabled by the client.`);
                 }
             },
-                `Failed to start the connection: Error: Unable to connect to the server with any of the available transports. ${HttpTransportType[requestedTransport]} failed: Error: '${HttpTransportType[requestedTransport]}' is disabled by the client.`,
+            `Failed to start the connection: Error: Unable to connect to the server with any of the available transports. ${HttpTransportType[requestedTransport]} failed: Error: '${HttpTransportType[requestedTransport]}' is disabled by the client.`);
-                `Failed to start the transport '${HttpTransportType[requestedTransport]}': Error: '${HttpTransportType[requestedTransport]}' is disabled by the client.`);
         });
     });