diff --git a/clients/ts/signalr/spec/LongPollingTransport.spec.ts b/clients/ts/signalr/spec/LongPollingTransport.spec.ts index 8023ab27d4..29e7fb9505 100644 --- a/clients/ts/signalr/spec/LongPollingTransport.spec.ts +++ b/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); + }); + } }); \ No newline at end of file diff --git a/clients/ts/signalr/src/LongPollingTransport.ts b/clients/ts/signalr/src/LongPollingTransport.ts index b2b456f8a2..6fdc830320 100644 --- a/clients/ts/signalr/src/LongPollingTransport.ts +++ b/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; @@ -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, 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 || ""}`); @@ -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. - if (!this.pollAbort.aborted) { + // Abort the poll after the shutdown timeout if the server doesn't stop the poll. + 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); }