Merge pull request #2195 from aspnet/anurse/clean-up-shutdown-timer

clients/ts/signalr/spec/LongPollingTransport.spec.ts

@@ -6,7 +6,9 @@ import { LongPollingTransport } from "../src/LongPollingTransport";
 import { ConsoleLogger } from "../src/Utils";
 
 import { TestHttpClient } from "./TestHttpClient";
-import { asyncit as it, PromiseSource } from "./Utils";
+import { asyncit as it, PromiseSource, delay } from "./Utils";
+import { HttpError, TimeoutError } from "../src/Errors";
+import { AbortSignal } from "../src/AbortController";
 
 describe("LongPollingTransport", () => {
     it("shuts down poll after timeout even if server doesn't shut it down on receiving the DELETE", async () => {
@@ -48,6 +50,8 @@ describe("LongPollingTransport", () => {
                     return new HttpResponse(200);
                 } else {
                     await deleteReceived.promise;
+                    // Force the shutdown timer to be registered by not returning inline
+                    await delay(10);
                     pollCompleted.resolve();
                     return new HttpResponse(204);
                 }
@@ -56,7 +60,7 @@ describe("LongPollingTransport", () => {
                 deleteReceived.resolve();
                 return new HttpResponse(202);
             });
-        const transport = new LongPollingTransport(client, null, NullLogger.instance, false);
+        const transport = new LongPollingTransport(client, null, NullLogger.instance, false, 100);
 
         await transport.connect("http://example.com", TransferFormat.Text);
         await transport.stop();
@@ -64,4 +68,55 @@ describe("LongPollingTransport", () => {
         // This should complete, because the DELETE request triggers it to stop.
         await pollCompleted.promise;
     });
+
+    for (const result of [200, 204, 300, new HttpError("Boom", 500), new TimeoutError()]) {
+        const resultName = typeof result === "number" ? result.toString() : result.constructor.name;
+        it(`does not fire shutdown timer when poll terminates with ${resultName}`, async () => {
+            let firstPoll = true;
+            const deleteReceived = new PromiseSource();
+            const pollCompleted = new PromiseSource();
+            const client = new TestHttpClient()
+                .on("GET", async (r) => {
+                    if (firstPoll) {
+                        firstPoll = false;
+                        return new HttpResponse(200);
+                    } else {
+                        await deleteReceived.promise;
+                        // Force the shutdown timer to be registered by not returning inline
+                        await delay(10);
+                        pollCompleted.resolve();
+
+                        if (typeof result === "number") {
+                            return new HttpResponse(result);
+                        } else {
+                            throw result;
+                        }
+                    }
+                })
+                .on("DELETE", (r) => {
+                    deleteReceived.resolve();
+                    return new HttpResponse(202);
+                });
+            const logMessages: string[] = [];
+            const transport = new LongPollingTransport(client, null, {
+                log(level: LogLevel, message: string) {
+                    logMessages.push(message);
+                },
+            }, false, 100);
+
+            await transport.connect("http://example.com", TransferFormat.Text);
+            await transport.stop();
+
+            // This should complete, because the DELETE request triggers it to stop.
+            await pollCompleted.promise;
+
+            // Wait for the shutdown timeout to elapse
+            // This can be much cleaner when we port to Jest because it has a built-in set of
+            // fake timers!
+            await delay(150);
+
+            // The pollAbort token should be left unaborted because we shut down gracefully.
+            expect(transport.pollAborted).toBe(false);
+        });
+    }
 });

clients/ts/signalr/src/LongPollingTransport.ts

@@ -10,6 +10,7 @@ import { Arg, getDataDetail, sendMessage } from "./Utils";
 
 const SHUTDOWN_TIMEOUT = 5 * 1000;
 
+// Not exported from 'index', this type is internal.
 export class LongPollingTransport implements ITransport {
     private readonly httpClient: HttpClient;
     private readonly accessTokenFactory: () => string | Promise<string>;
@@ -22,6 +23,12 @@ export class LongPollingTransport implements ITransport {
     private shutdownTimer: any; // We use 'any' because this is an object in NodeJS. But it still gets passed to clearTimeout, so it doesn't really matter
     private shutdownTimeout: number;
     private running: boolean;
+    private stopped: boolean;
+
+    // This is an internal type, not exported from 'index' so this is really just internal.
+    public get pollAborted() {
+        return this.pollAbort.aborted;
+    }
 
     constructor(httpClient: HttpClient, accessTokenFactory: () => string | Promise<string>, logger: ILogger, logMessageContent: boolean, shutdownTimeout?: number) {
         this.httpClient = httpClient;
@@ -108,9 +115,6 @@ export class LongPollingTransport implements ITransport {
                     if (response.statusCode === 204) {
                         this.logger.log(LogLevel.Information, "(LongPolling transport) Poll terminated by server");
 
-                        // If we were on a timeout waiting for shutdown, unregister it.
-                        clearTimeout(this.shutdownTimer);
-
                         this.running = false;
                     } else if (response.statusCode !== 200) {
                         this.logger.log(LogLevel.Error, `(LongPolling transport) Unexpected response code: ${response.statusCode}`);
@@ -147,6 +151,14 @@ export class LongPollingTransport implements ITransport {
                 }
             }
         } finally {
+            // Indicate that we've stopped so the shutdown timer doesn't get registered.
+            this.stopped = true;
+
+            // Clean up the shutdown timer if it was registered
+            if (this.shutdownTimer) {
+                clearTimeout(this.shutdownTimer);
+            }
+
             // Fire our onclosed event
             if (this.onclose) {
                 this.logger.log(LogLevel.Trace, `(LongPolling transport) Firing onclose event. Error: ${closeError || "<undefined>"}`);
@@ -179,10 +191,12 @@ export class LongPollingTransport implements ITransport {
 
             this.logger.log(LogLevel.Trace, "(LongPolling transport) DELETE request accepted.");
         } finally {
-            // Abort the poll after 5 seconds if the server doesn't stop it.
+            // Abort the poll after the shutdown timeout if the server doesn't stop the poll.
-            if (!this.pollAbort.aborted) {
+            if (!this.stopped) {
                 this.shutdownTimer = setTimeout(() => {
                     this.logger.log(LogLevel.Warning, "(LongPolling transport) server did not terminate after DELETE request, canceling poll.");
+
+                    // Abort any outstanding poll
                     this.pollAbort.abort();
                 }, this.shutdownTimeout);
             }