From 774dcb2dbb6cd6bed77699041c9b2a2eae3d05f9 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 10:44:31 -0700 Subject: [PATCH 01/33] WIP: Add watchdog --- package-lock.json | 36 +-- package.json | 2 +- src/directLineStreaming.ts | 268 +++++++++++++----- ...cketClientWithWatchdog.integration.test.ts | 77 +++++ .../WebSocketClientWithWatchdog.test.ts | 138 +++++++++ src/streaming/WebSocketClientWithWatchdog.ts | 54 ++++ src/streaming/__setup__/waitFor.ts | 71 +++++ src/streaming/sleep.test.ts | 73 +++++ src/streaming/sleep.ts | 27 ++ src/streaming/watchREST.test.ts | 216 ++++++++++++++ src/streaming/watchREST.ts | 93 ++++++ 11 files changed, 965 insertions(+), 90 deletions(-) create mode 100644 src/streaming/WebSocketClientWithWatchdog.integration.test.ts create mode 100644 src/streaming/WebSocketClientWithWatchdog.test.ts create mode 100644 src/streaming/WebSocketClientWithWatchdog.ts create mode 100644 src/streaming/__setup__/waitFor.ts create mode 100644 src/streaming/sleep.test.ts create mode 100644 src/streaming/sleep.ts create mode 100644 src/streaming/watchREST.test.ts create mode 100644 src/streaming/watchREST.ts diff --git a/package-lock.json b/package-lock.json index e273069a..f422fd5f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -54,7 +54,7 @@ "restify": "^11.0.0", "rimraf": "^3.0.2", "simple-update-in": "^2.2.0", - "typescript": "^4.3.5", + "typescript": "^4.9.5", "webpack": "^5.76.2", "webpack-cli": "^4.7.2", "webpack-stats-plugin": "^1.0.3", @@ -2804,21 +2804,21 @@ "dev": true }, "node_modules/@sinonjs/commons": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-2.0.0.tgz", - "integrity": "sha512-uLa0j859mMrg2slwQYdO/AkrOfmH+X6LTVmNTS9CqexuE2IvVORIkSpJLqePAbEnKJ77aMmCwr1NUZ57120Xcg==", + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-3.0.0.tgz", + "integrity": "sha512-jXBtWAF4vmdNmZgD5FoKsVLv3rPgDnLgPbU84LIJ3otV44vJlDRokVng5v8NFJdCf/da9legHcKaRuZs4L7faA==", "dev": true, "dependencies": { "type-detect": "4.0.8" } }, "node_modules/@sinonjs/fake-timers": { - "version": "10.0.2", - "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-10.0.2.tgz", - "integrity": "sha512-SwUDyjWnah1AaNl7kxsa7cfLhlTYoiyhDAIgyh+El30YvXs/o7OLXpYH88Zdhyx9JExKrmHDJ+10bwIcY80Jmw==", + "version": "10.3.0", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-10.3.0.tgz", + "integrity": "sha512-V4BG07kuYSUkTCSBHG8G8TNhM+F19jXFWnQtzj+we8DrkpSBCee9Z3Ms8yiGer/dlmhe35/Xdgyo3/0rQKg7YA==", "dev": true, "dependencies": { - "@sinonjs/commons": "^2.0.0" + "@sinonjs/commons": "^3.0.0" } }, "node_modules/@testing-library/dom": { @@ -10169,7 +10169,7 @@ "node_modules/lodash.set": { "version": "4.3.2", "resolved": "https://registry.npmjs.org/lodash.set/-/lodash.set-4.3.2.tgz", - "integrity": "sha1-2HV7HagH3eJIFrDWqEvqGnYjCyM=", + "integrity": "sha512-4hNPN5jlm/N/HLMCO43v8BXKq9Z7QdAGc/VGrRD61w8gN9g/6jF9A4L1pbUgBLCffi0w9VsXfTOij5x8iTyFvg==", "dev": true }, "node_modules/lru-cache": { @@ -10486,9 +10486,9 @@ } }, "node_modules/nock/node_modules/debug": { - "version": "4.3.2", - "resolved": "https://registry.npmjs.org/debug/-/debug-4.3.2.tgz", - "integrity": "sha512-mOp8wKcvj7XxC78zLgw/ZA+6TSgkoE2C/ienthhRD298T7UNwAg9diBpLRxC0mOezLl4B0xV7M0cCO6P/O0Xhw==", + "version": "4.3.4", + "resolved": "https://registry.npmjs.org/debug/-/debug-4.3.4.tgz", + "integrity": "sha512-PRWFHuSU3eDtQJPvnNY7Jcket1j0t5OuOsFzPPzsekD52Zl8qUfFIPEiswXqIvHWGVHOgX+7G/vCNNhehwxfkQ==", "dev": true, "dependencies": { "ms": "2.1.2" @@ -10530,17 +10530,17 @@ "node_modules/node-fetch/node_modules/tr46": { "version": "0.0.3", "resolved": "https://registry.npmjs.org/tr46/-/tr46-0.0.3.tgz", - "integrity": "sha1-gYT9NH2snNwYWZLzpmIuFLnZq2o=" + "integrity": "sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw==" }, "node_modules/node-fetch/node_modules/webidl-conversions": { "version": "3.0.1", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", - "integrity": "sha1-JFNCdeKnvGvnvIZhHMFq4KVlSHE=" + "integrity": "sha512-2JAn3z8AR6rjK8Sm8orRC0h/bcl/DqL7tRPdGZ4I1CjdF+EaMLmYxBHyXuKL849eucPFhvBoxMsflfOb8kxaeQ==" }, "node_modules/node-fetch/node_modules/whatwg-url": { "version": "5.0.0", "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-5.0.0.tgz", - "integrity": "sha1-lmRU6HZUYuN2RNNib2dCzotwll0=", + "integrity": "sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==", "dependencies": { "tr46": "~0.0.3", "webidl-conversions": "^3.0.0" @@ -12813,9 +12813,9 @@ } }, "node_modules/typescript": { - "version": "4.3.5", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.3.5.tgz", - "integrity": "sha512-DqQgihaQ9cUrskJo9kIyW/+g0Vxsk8cDtZ52a3NGh0YNTfpUSArXSohyUGnvbPazEPLu398C0UxmKSOrPumUzA==", + "version": "4.9.5", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.9.5.tgz", + "integrity": "sha512-1FXk9E2Hm+QzZQ7z+McJiHL4NW1F2EzMu9Nq9i3zAaGqibafqYwCVU6WyWAuyQRRzOlxou8xZSyXLEN8oKj24g==", "dev": true, "bin": { "tsc": "bin/tsc", diff --git a/package.json b/package.json index ed05c3f0..92f05cf7 100644 --- a/package.json +++ b/package.json @@ -71,7 +71,7 @@ "restify": "^11.0.0", "rimraf": "^3.0.2", "simple-update-in": "^2.2.0", - "typescript": "^4.3.5", + "typescript": "^4.9.5", "webpack": "^5.76.2", "webpack-cli": "^4.7.2", "webpack-stats-plugin": "^1.0.3", diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index 91a03752..e6f8fb8f 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -8,30 +8,57 @@ import * as BFSE from 'botframework-streaming'; import createDeferred from './createDeferred'; import fetch from 'cross-fetch'; -import type { Deferred } from './createDeferred'; +import watchREST from './streaming/watchREST'; +import WebSocketClientWithWatchdog from './streaming/WebSocketClientWithWatchdog'; -import { - Activity, - ConnectionStatus, - Conversation, - IBotConnection, - Media, - Message -} from './directLine'; +import type { Deferred } from './createDeferred'; +import { Activity, ConnectionStatus, Conversation, IBotConnection, Media, Message } from './directLine'; const DIRECT_LINE_VERSION = 'DirectLine/3.0'; const MAX_RETRY_COUNT = 3; const refreshTokenLifetime = 30 * 60 * 1000; //const refreshTokenLifetime = 5000; -const timeout = 20 * 1000; +// const timeout = 20 * 1000; const refreshTokenInterval = refreshTokenLifetime / 2; interface DirectLineStreamingOptions { - token: string, - conversationId?: string, - domain: string, + token: string; + conversationId?: string; + domain: string; // Attached to all requests to identify requesting agent. - botAgent?: string + botAgent?: string; + + /** + * Sets the connection liveness probe. + * + * When the probe detects any connection issues, the bot connection will be closed and treated as an error. + * + * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` did not emit `error` event timely in case of connection issues. + * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. + * + * There are 2 ways to set the probe: `object` or `function`. + * + * When the probe is an object: + * + * - `url` is the URL of a REST long-polling API. The REST API must keep the connection for a period of time and returns HTTP 2xx when it end. + * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. + * - `pingInterval` is the time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. + * It must be shorter than the time the API would keep the connection. + * + * When the probe is a function: + * + * - The function will be called when a connection is being established. Thus, a new liveness probe is needed. + * - The returned `AbortSignal` should be aborted as soon as the probe detects any connection issues. + * - The function should create a new probe on every call and probe should not be reused. + * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. + * - At any point of time, there should be no more than 1 probe active. The chat adapter is expected to signal the release of probe before requesting for a new one. + */ + watchdog?: + | ((init: { signal: AbortSignal }) => AbortSignal) + | { + pingInterval?: number; + url: string | URL; + }; } class StreamHandler implements BFSE.RequestHandler { @@ -68,9 +95,9 @@ class StreamHandler implements BFSE.RequestHandler { const attachments = [...activity.attachments]; let stream: BFSE.ContentStream; - while (stream = streams.shift()) { + while ((stream = streams.shift())) { const attachment = await stream.readAsString(); - const dataUri = "data:text/plain;base64," + attachment; + const dataUri = 'data:text/plain;base64,' + attachment; attachments.push({ contentType: stream.contentType, contentUrl: dataUri }); } @@ -87,8 +114,8 @@ class StreamHandler implements BFSE.RequestHandler { } public flush() { - this.connectionStatus$.subscribe(cs => { }) - this.activityQueue.forEach((a) => this.subscriber.next(a)); + this.connectionStatus$.subscribe(() => {}); + this.activityQueue.forEach(a => this.subscriber.next(a)); this.activityQueue = []; } @@ -114,7 +141,33 @@ export class DirectLineStreaming implements IBotConnection { private _botAgent = ''; + #watchdog: + | ((init: { signal: AbortSignal }) => AbortSignal) + | { + pingInterval?: number; + url: string | URL; + } + | undefined; + constructor(options: DirectLineStreamingOptions) { + // Verifies options.watchdog. + const watchdog = options?.watchdog; + + if ( + !( + typeof watchdog === 'undefined' || + typeof watchdog === 'function' || + ((typeof watchdog.pingInterval === 'number' || typeof watchdog.pingInterval === 'undefined') && + (typeof watchdog.url === 'string' || watchdog.url instanceof URL)) + ) + ) { + throw new Error( + 'botframework-directlinejs: "watchdog" option must be either a function returning an AbortSignal, an object, or undefined.' + ); + } + + this.#watchdog = options?.watchdog; + this.token = options.token; this.refreshToken().catch(() => { @@ -142,7 +195,9 @@ export class DirectLineStreaming implements IBotConnection { this.connectWithRetryAsync(); } - public reconnect({ conversationId, token } : Conversation) { + public reconnect({ conversationId, token }: Conversation) { + console.log('DLASE: reconnect', conversationId); + if (this.connectionStatus$.getValue() === ConnectionStatus.Ended) { throw new Error('Connection has ended.'); } @@ -165,16 +220,16 @@ export class DirectLineStreaming implements IBotConnection { private commonHeaders() { return { - "Authorization": `Bearer ${this.token}`, - "x-ms-bot-agent": this._botAgent + Authorization: `Bearer ${this.token}`, + 'x-ms-bot-agent': this._botAgent }; } private getBotAgent(customAgent: string = ''): string { - let clientAgent = 'directlineStreaming' + let clientAgent = 'directlineStreaming'; if (customAgent) { - clientAgent += `; ${customAgent}` + clientAgent += `; ${customAgent}`; } return `${DIRECT_LINE_VERSION} (${clientAgent})`; @@ -184,14 +239,14 @@ export class DirectLineStreaming implements IBotConnection { await this.waitUntilOnline(); let numberOfAttempts = 0; - while(numberOfAttempts < MAX_RETRY_COUNT) { + while (numberOfAttempts < MAX_RETRY_COUNT) { numberOfAttempts++; await new Promise(r => setTimeout(r, refreshTokenInterval)); try { - const res = await fetch(`${this.domain}/tokens/refresh`, {method: "POST", headers: this.commonHeaders()}); + const res = await fetch(`${this.domain}/tokens/refresh`, { method: 'POST', headers: this.commonHeaders() }); if (res.ok) { numberOfAttempts = 0; - const {token} = await res.json(); + const { token } = await res.json(); this.token = token; } else { if (res.status === 403 || res.status === 403) { @@ -201,44 +256,50 @@ export class DirectLineStreaming implements IBotConnection { console.warn(`Refresh attempt #${numberOfAttempts} failed: ${res.status} ${res.statusText}`); } } - } catch(e) { + } catch (e) { console.warn(`Refresh attempt #${numberOfAttempts} threw an exception: ${e}`); } } - console.error("Retries exhausted"); + console.error('Retries exhausted'); this.streamConnection.disconnect(); } postActivity(activity: Activity) { - if (this.connectionStatus$.value === ConnectionStatus.Ended || this.connectionStatus$.value === ConnectionStatus.FailedToConnect) { + if ( + this.connectionStatus$.value === ConnectionStatus.Ended || + this.connectionStatus$.value === ConnectionStatus.FailedToConnect + ) { return Observable.throw(new Error('Connection is closed')); } - if (activity.type === "message" && activity.attachments && activity.attachments.length > 0) { + if (activity.type === 'message' && activity.attachments && activity.attachments.length > 0) { return this.postMessageWithAttachments(activity); } const resp$ = Observable.create(async subscriber => { - const request = BFSE.StreamingRequest.create('POST', '/v3/directline/conversations/' + this.conversationId + '/activities'); + const request = BFSE.StreamingRequest.create( + 'POST', + '/v3/directline/conversations/' + this.conversationId + '/activities' + ); request.setBody(JSON.stringify(activity)); try { const resp = await this.streamConnection.send(request); - if (resp.statusCode !== 200) throw new Error("PostActivity returned " + resp.statusCode); + if (resp.statusCode !== 200) throw new Error('PostActivity returned ' + resp.statusCode); const numberOfStreams = resp.streams.length; - if (numberOfStreams !== 1) throw new Error("Expected one stream but got " + numberOfStreams) + if (numberOfStreams !== 1) throw new Error('Expected one stream but got ' + numberOfStreams); const idString = await resp.streams[0].readAsString(); - const {Id : id} = JSON.parse(idString); + const { Id: id } = JSON.parse(idString); subscriber.next(id); return subscriber.complete(); - } catch(e) { - // If there is a network issue then its handled by - // the disconnectionHandler. Everything else can - // be retried - console.warn(e); - this.streamConnection.disconnect(); - return subscriber.error(e); + } catch (e) { + // If there is a network issue then its handled by + // the disconnectionHandler. Everything else can + // be retried + console.warn(e); + this.streamConnection.disconnect(); + return subscriber.error(e); } }); return resp$; @@ -247,19 +308,21 @@ export class DirectLineStreaming implements IBotConnection { private postMessageWithAttachments(message: Message) { const { attachments, ...messageWithoutAttachments } = message; - return Observable.create( subscriber => { + return Observable.create(subscriber => { const httpContentList = []; (async () => { try { - const arrayBuffers = await Promise.all(attachments.map(async attachment => { - const media = attachment as Media; - const res = await fetch(media.contentUrl); - if (res.ok) { - return { arrayBuffer: await res.arrayBuffer(), media }; - } else { - throw new Error('...'); - } - })); + const arrayBuffers = await Promise.all( + attachments.map(async attachment => { + const media = attachment as Media; + const res = await fetch(media.contentUrl); + if (res.ok) { + return { arrayBuffer: await res.arrayBuffer(), media }; + } else { + throw new Error('...'); + } + }) + ); arrayBuffers.forEach(({ arrayBuffer, media }) => { const buffer = Buffer.from(arrayBuffer); @@ -273,18 +336,23 @@ export class DirectLineStreaming implements IBotConnection { const request = BFSE.StreamingRequest.create('PUT', url); const activityStream = new BFSE.SubscribableStream(); activityStream.write(JSON.stringify(messageWithoutAttachments), 'utf-8'); - request.addStream(new BFSE.HttpContent({ type: "application/vnd.microsoft.activity", contentLength: activityStream.length }, activityStream)); + request.addStream( + new BFSE.HttpContent( + { type: 'application/vnd.microsoft.activity', contentLength: activityStream.length }, + activityStream + ) + ); httpContentList.forEach(e => request.addStream(e)); const resp = await this.streamConnection.send(request); if (resp.streams && resp.streams.length !== 1) { subscriber.error(new Error(`Invalid stream count ${resp.streams.length}`)); } else { - const {Id: id} = await resp.streams[0].readAsJson<{Id: string}>(); + const { Id: id } = await resp.streams[0].readAsJson<{ Id: string }>(); subscriber.next(id); subscriber.complete(); } - } catch(e) { + } catch (e) { subscriber.error(e); } })(); @@ -293,50 +361,96 @@ export class DirectLineStreaming implements IBotConnection { private async waitUntilOnline() { return new Promise((resolve, reject) => { - this.connectionStatus$.subscribe((cs) => { - if (cs === ConnectionStatus.Online) return resolve(); - }, - (e) => reject(e)); - }) + this.connectionStatus$.subscribe( + cs => { + if (cs === ConnectionStatus.Online) { + return resolve(); + } + }, + e => reject(e) + ); + }); } private async connectAsync() { const re = new RegExp('^http(s?)'); - if (!re.test(this.domain)) throw ("Domain must begin with http or https"); - const params = {token: this.token}; - if (this.conversationId) params['conversationId'] = this.conversationId; + + if (!re.test(this.domain)) { + throw 'Domain must begin with http or https'; + } + + const params = { token: this.token }; + + if (this.conversationId) { + params['conversationId'] = this.conversationId; + } + + const abortController = new AbortController(); const urlSearchParams = new URLSearchParams(params).toString(); const wsUrl = `${this.domain.replace(re, 'ws$1')}/conversations/connect?${urlSearchParams}`; // This promise will resolve when it is disconnected. return new Promise(async (resolve, reject) => { try { - this.streamConnection = new BFSE.WebSocketClient({ + const watchdog: AbortSignal | undefined = + typeof this.#watchdog === 'function' + ? this.#watchdog({ signal: abortController.signal }) + : typeof this.#watchdog === 'undefined' + ? undefined + : watchREST(this.#watchdog.url, { + pingInterval: this.#watchdog.pingInterval, + signal: abortController.signal + }); + + this.streamConnection = new WebSocketClientWithWatchdog({ + disconnectionHandler: resolve, + requestHandler: { + processRequest: streamingRequest => { + // If `streamConnection` is still current, allow call to `processRequest()`, otherwise, ignore calls to `processRequest()`. + // This prevents zombie connections from sending us requests. + if (abortController.signal.aborted) { + throw new Error('Cannot process streaming request, `streamingConnection` should be disconnected.'); + } + + return this.theStreamHandler.processRequest(streamingRequest); + } + }, url: wsUrl, - requestHandler: this.theStreamHandler, - disconnectionHandler: (e) => resolve(e) + watchdog }); this.queueActivities = true; + await this.streamConnection.connect(); + const request = BFSE.StreamingRequest.create('POST', '/v3/directline/conversations'); const response = await this.streamConnection.send(request); - if (response.statusCode !== 200) throw new Error("Connection response code " + response.statusCode); - if (response.streams.length !== 1) throw new Error("Expected 1 stream but got " + response.streams.length); + + if (response.statusCode !== 200) { + throw new Error('Connection response code ' + response.statusCode); + } + + if (response.streams.length !== 1) { + throw new Error('Expected 1 stream but got ' + response.streams.length); + } + const responseString = await response.streams[0].readAsString(); const conversation = JSON.parse(responseString); + this.conversationId = conversation.conversationId; this.connectionStatus$.next(ConnectionStatus.Online); // Wait until DL consumers have had a chance to be notified // of the connection status change. + // This is specific to RxJS implementation of observable, which calling subscribe() after next() will still get the value. await this.waitUntilOnline(); + this.theStreamHandler.flush(); this.queueActivities = false; - } catch(e) { + } catch (e) { reject(e); } - }); + }).finally(() => abortController.abort()); } private async connectWithRetryAsync() { @@ -357,12 +471,21 @@ export class DirectLineStreaming implements IBotConnection { const start = Date.now(); try { + console.log('DLASE: connectAsync', numRetries); + // This promise will reject/resolve when disconnected. await this.connectAsync(); - } catch (err) {} + } catch (err) { + console.error(err); + } + + console.log('DLASE: connection broken'); // If someone call end() to break the connection, we will never listen to any reconnect(). if (this.connectionStatus$.getValue() === ConnectionStatus.Ended) { + console.log('DLASE: ended'); + + // This is the only place the loop in this function will be broke. return; } @@ -376,19 +499,22 @@ export class DirectLineStreaming implements IBotConnection { // - we should reset the retry counter, and; // - we should reconnect immediately. if (60000 < Date.now() - start) { + console.log('DLASE: good connection, reset retry and no delay'); numRetries = MAX_RETRY_COUNT; } else if (numRetries > 0) { // Sleep only if we are doing retry. Otherwise, we are going to break the loop and signal FailedToConnect. + console.log('DLASE: poor connection, delay'); await new Promise(r => setTimeout(r, this.getRetryDelay())); } } + console.log('DLASE: failed to connect'); // TODO: [TEST] Make sure FailedToConnect is reported immediately after last disconnection, should be no getRetryDelay(). // Failed to reconnect after multiple retries. this.connectionStatus$.next(ConnectionStatus.FailedToConnect); } - throw (new Error(`Failed to connect after ${MAX_RETRY_COUNT} attempts`)); + // Note: No code will hit this line. } // Returns the delay duration in milliseconds diff --git a/src/streaming/WebSocketClientWithWatchdog.integration.test.ts b/src/streaming/WebSocketClientWithWatchdog.integration.test.ts new file mode 100644 index 00000000..4d146124 --- /dev/null +++ b/src/streaming/WebSocketClientWithWatchdog.integration.test.ts @@ -0,0 +1,77 @@ +import type { WebSocketClient } from 'botframework-streaming'; +import type OriginalWebSocketClientWithWatchdog from './WebSocketClientWithWatchdog'; + +// Mocked modules are available across the test file. They cannot be unmocked. +// Thus, they are more-or-less similar to import/require. +jest.mock('../../node_modules/botframework-streaming/lib/webSocket/nodeWebSocket', () => ({ + __esmodule: true, + NodeWebSocket: class { + connect() {} + setOnCloseHandler() {} + setOnErrorHandler() {} + setOnMessageHandler() {} + } +})); + +type WebSocketClientInit = ConstructorParameters[0]; + +const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); +const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; +const url: string = 'wss://dummy/'; + +let client: WebSocketClient; +let watchdog: AbortController; + +beforeEach(() => { + watchdog = new AbortController(); + + let WebSocketClientWithWatchdog: typeof OriginalWebSocketClientWithWatchdog; + + WebSocketClientWithWatchdog = require('./WebSocketClientWithWatchdog').default; + + client = new WebSocketClientWithWatchdog({ + disconnectionHandler, + requestHandler, + url, + watchdog: watchdog.signal + }); + + // Spy on all `console.warn()`. + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => jest.restoreAllMocks()); + +test('should not call disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(0)); + +describe('call connect()', () => { + beforeEach(() => client.connect()); + + describe('call disconnect()', () => { + beforeEach(() => client.disconnect()); + + // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. + test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); + + describe('followed by aborting watchdog', () => { + beforeEach(() => watchdog.abort()); + + // After disconnected() is called, there should be no extra calls for aborting the signal. + test('should have no extra calls to disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(2)); + }); + }); + + describe('abort watchdog', () => { + beforeEach(() => watchdog.abort()); + + // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. + test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); + + describe('then call disconnect()', () => { + beforeEach(() => client.disconnect()); + + // After the signal is aborted, there should be no extra calls for calling disconnect(). + test('should have no extra calls to disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(2)); + }); + }); +}); diff --git a/src/streaming/WebSocketClientWithWatchdog.test.ts b/src/streaming/WebSocketClientWithWatchdog.test.ts new file mode 100644 index 00000000..b6eeff72 --- /dev/null +++ b/src/streaming/WebSocketClientWithWatchdog.test.ts @@ -0,0 +1,138 @@ +import type { WebSocketClient } from 'botframework-streaming'; +import type ActualWebSocketClientWithWatchdog from './WebSocketClientWithWatchdog'; + +// Mocked modules are available across the test file. They cannot be unmocked. +// Thus, they are more-or-less similar to import/require. +jest.mock('botframework-streaming', () => ({ + __esmodule: true, + WebSocketClient: class WebSocketClient { + constructor({ disconnectionHandler, requestHandler, url }: WebSocketClientInit) { + this.#disconnectionHandler = disconnectionHandler; + this.#requestHandler = requestHandler; + this.#url = url; + + // Set up mocks. + this.#connect = jest.fn(() => Promise.resolve()); + this.#disconnect = jest.fn(() => this.#disconnectionHandler?.('disconnect() is called')); + } + + #connect: () => Promise; + #disconnect: () => void; + #disconnectionHandler: WebSocketClientInit['disconnectionHandler']; + #requestHandler: WebSocketClientInit['requestHandler']; + #url: string; + + connect(): Promise { + return this.#connect(); + } + + disconnect(): void { + return this.#disconnect(); + } + + get __test__connect(): () => Promise { + return this.#connect; + } + + get __test__disconnect(): () => void { + return this.#disconnect; + } + + get __test__disconnectionHandler(): WebSocketClientInit['disconnectionHandler'] { + return this.#disconnectionHandler; + } + + get __test__requestHandler(): WebSocketClientInit['requestHandler'] { + return this.#requestHandler; + } + + get __test__url(): WebSocketClientInit['url'] { + return this.#url; + } + } +})); + +type WebSocketClientInit = ConstructorParameters[0]; + +const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); +const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; +const url: string = 'wss://dummy/'; + +let client: WebSocketClient; +let watchdog: AbortController; + +beforeEach(() => { + watchdog = new AbortController(); + + let WebSocketClientWithWatchdog: typeof ActualWebSocketClientWithWatchdog; + + WebSocketClientWithWatchdog = require('./WebSocketClientWithWatchdog').default; + + client = new WebSocketClientWithWatchdog({ + disconnectionHandler, + requestHandler, + url, + watchdog: watchdog.signal + }); + + // Spy on all `console.warn()`. + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => jest.restoreAllMocks()); + +describe('constructor', () => { + test('should pass `disconnectionHandler`', () => + expect(client['__test__disconnectionHandler']).toBe(disconnectionHandler)); + test('should pass `requestHandler`', () => expect(client['__test__requestHandler']).toBe(requestHandler)); + test('should pass `url`', () => expect(client['__test__url']).toBe(url)); +}); + +describe('connect()', () => { + test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); + + describe('when called', () => { + beforeEach(() => client.connect()); + + test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); + + describe('twice', () => { + beforeEach(() => client.connect()); + + test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); + test('should warn "connect() can only be called once"', () => + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once'))); + test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); + }); + + describe('then abort watchdog', () => { + beforeEach(() => watchdog.abort()); + + test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + + describe('when connect() is called again', () => { + beforeEach(() => client.connect()); + + test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); + test('should warn "connect() can only be called once"', () => + expect(console.warn).toHaveBeenNthCalledWith( + 1, + expect.stringContaining('connect() can only be called once') + )); + test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); + }); + }); + }); +}); + +describe('watchdog is aborted then call connect()', () => { + beforeEach(() => { + watchdog.abort(); + client.connect(); + }); + + test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); + test('should warn "watchdog is aborted before connect()"', () => + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('watchdog is aborted before connect()'))); + test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); +}); diff --git a/src/streaming/WebSocketClientWithWatchdog.ts b/src/streaming/WebSocketClientWithWatchdog.ts new file mode 100644 index 00000000..a46ffc7d --- /dev/null +++ b/src/streaming/WebSocketClientWithWatchdog.ts @@ -0,0 +1,54 @@ +import { WebSocketClient } from 'botframework-streaming'; + +import type { RequestHandler } from 'botframework-streaming'; + +type WebSocketClientWithWatchdogInit = { + /** + * Gets or sets the observer function for disconnection or error sending/receiving through WebSocket. + * + * Note: This function could be called multiple times, the callee is expected to ignore subsequent calls. + */ + disconnectionHandler?: (message: string) => void; + requestHandler: RequestHandler; + url: string; + watchdog?: AbortSignal; +}; + +export default class WebSocketClientWithWatchdog extends WebSocketClient { + constructor({ disconnectionHandler, requestHandler, url, watchdog }: WebSocketClientWithWatchdogInit) { + super({ + disconnectionHandler, + requestHandler, + url + }); + + this.#watchdog = watchdog; + } + + #connectCalled: boolean = false; + #watchdog: WebSocketClientWithWatchdogInit['watchdog']; + + // TODO: Better, the `watchdog` should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. + // `BrowserWebSocket` is where it creates `WebSocket` object. + // The `watchdog` object should accompany `WebSocket` and forcibly close it on abort. + connect(): Promise { + if (this.#connectCalled) { + console.warn('botframework-directlinejs: connect() can only be called once.'); + + return; + } else if (this.#watchdog?.aborted) { + console.warn('botframework-directlinejs: watchdog is aborted before connect().'); + + return; + } + + this.#connectCalled = true; + + // Note: It is expected disconnectionHandler() will be called multiple times. + // If we call disconnect(), both sender/receiver will call super.onConnectionDisconnected(), + // which in turn, call disconnectionHandler() twice, all from a single disconnect() call. + this.#watchdog?.addEventListener('abort', () => this.disconnect(), { once: true }); + + return super.connect(); + } +} diff --git a/src/streaming/__setup__/waitFor.ts b/src/streaming/__setup__/waitFor.ts new file mode 100644 index 00000000..057f77e6 --- /dev/null +++ b/src/streaming/__setup__/waitFor.ts @@ -0,0 +1,71 @@ +const DEFAULT_DURATION = 1000; +const DEFAULT_INTERVAL = 50; + +function isPromise(value: T | Promise): value is Promise { + return value && typeof (value as Promise).then === 'function'; +} + +export default function waitFor( + fn: () => Promise | T, + { + duration, + interval + }: { + duration?: number; + interval?: number; + } = {} +): Promise { + let runInterval: ReturnType; + let runTimeout: ReturnType; + + const promise = new Promise((resolve, reject) => { + let lastError: any; + let ready = true; + + const run = () => { + if (!ready) { + return; + } + + ready = false; + + let returnValue; + + try { + returnValue = fn(); + } catch (error) { + lastError = error; + ready = true; + + return; + } + + if (isPromise(returnValue)) { + returnValue.then( + result => resolve(result), + error => { + lastError = error; + ready = true; + } + ); + } else { + resolve(returnValue); + } + }; + + runInterval = setInterval(run, interval || DEFAULT_INTERVAL); + + runTimeout = setTimeout(() => { + reject(lastError || new Error('timed out')); + }, duration || DEFAULT_DURATION); + + run(); + }).finally(() => { + clearInterval(runInterval); + clearTimeout(runTimeout); + }); + + promise.catch(() => {}); + + return promise; +} diff --git a/src/streaming/sleep.test.ts b/src/streaming/sleep.test.ts new file mode 100644 index 00000000..883f1c51 --- /dev/null +++ b/src/streaming/sleep.test.ts @@ -0,0 +1,73 @@ +import sleep from './sleep'; + +beforeEach(() => { + jest.useFakeTimers({ now: 0 }); +}); + +describe('sleep for 1s', () => { + let abortController: AbortController; + let now: number; + let promise: Promise; + + beforeEach(() => { + abortController = new AbortController(); + now = Date.now(); + promise = sleep(1000, { signal: abortController.signal }); + }); + + afterEach(() => { + promise.catch(() => {}); + }); + + test('should have 1 timer', () => expect(jest.getTimerCount()).toBe(1)); + + describe('when wake up', () => { + beforeEach(() => { + jest.runAllTimers(); + + return promise; + }); + + test('should passed 1s', () => expect(Date.now() - now).toBe(1000)); + }); + + describe('when aborted', () => { + beforeEach(() => abortController.abort()); + + test('should have no timers', () => expect(jest.getTimerCount()).toBe(0)); + test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); + }); + + describe('when 0.5s passed followed by abort', () => { + beforeEach(() => { + jest.advanceTimersByTime(500); + abortController.abort(); + }); + + test('should have no timers', () => expect(jest.getTimerCount()).toBe(0)); + test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); + }); +}); + +describe('abort before sleep for 1s', () => { + let abortController: AbortController; + let now: number; + let promise: Promise; + + beforeEach(() => { + abortController = new AbortController(); + abortController.abort(); + now = Date.now(); + promise = sleep(1000, { signal: abortController.signal }); + + jest.runAllTimers(); + }); + + afterEach(() => { + promise.catch(() => {}); + }); + + test('should have no timer', () => expect(jest.getTimerCount()).toBe(0)); + test('should not advance time', () => expect(Date.now()).toBe(now)); + test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); +}); diff --git a/src/streaming/sleep.ts b/src/streaming/sleep.ts new file mode 100644 index 00000000..1406367c --- /dev/null +++ b/src/streaming/sleep.ts @@ -0,0 +1,27 @@ +type Init = { + signal?: AbortSignal; +}; + +export default async function sleep(durationInMS: number = 100, init: Init = {}): Promise { + // Rectifying `durationInMS`. + durationInMS = Math.max(0, durationInMS); + + const { signal } = init; + + if (signal?.aborted) { + return Promise.reject(new Error('Aborted.')); + } + + let handleAbort: () => void; + let timeout: ReturnType; + + return new Promise((resolve, reject) => { + const handleAbort = () => reject(new Error('Aborted.')); + + timeout = setTimeout(resolve, durationInMS); + signal?.addEventListener('abort', handleAbort, { once: true }); + }).finally(() => { + clearTimeout(timeout); + signal?.removeEventListener('abort', handleAbort); + }); +} diff --git a/src/streaming/watchREST.test.ts b/src/streaming/watchREST.test.ts new file mode 100644 index 00000000..9882a48a --- /dev/null +++ b/src/streaming/watchREST.test.ts @@ -0,0 +1,216 @@ +import { createServer } from 'http'; +import express from 'express'; +import hasResolved from 'has-resolved'; + +import createDeferred from '../createDeferred'; +import waitFor from './__setup__/waitFor'; +import watchREST from './watchREST'; + +import type { Deferred } from '../createDeferred'; +import type { IncomingMessage, Server, ServerResponse } from 'http'; +import type { Request, Response } from 'express'; + +const pollFn = jest.fn, [Request, Response], any>(async (_req, res) => { + const callIndex = pollFn.mock.calls.length - 1; + + pollCallDeferred[callIndex]?.resolve(); + + res.chunkedEncoding = true; + res.status(200); + res.setHeader('cache-control', 'no-transform'); + res.write(' '); + + const handleClose = () => pollReturnDeferred[callIndex]?.reject(new Error('Socket closed.')); + + res.once('close', handleClose); + + setTimeout(() => { + res.off('close', handleClose); + res.end(); + + pollReturnDeferred[callIndex]?.resolve(); + }, 30_000); +}); + +function setup(): Promise void]> { + return new Promise(resolve => { + const app = express(); + + app.get('/poll', pollFn); + + const server = createServer(app); + + server.listen(() => { + const url = new URL('http://localhost/poll'); + const address = server.address(); + + if (typeof address === 'string') { + url.host = address; + } else if (address) { + url.hostname = address.address; + url.port = address.port + ''; + } + + resolve(Object.freeze([url, server, () => server.closeAllConnections()])); + }); + }); +} + +let abortController: AbortController; +let pollCallDeferred: Deferred[]; +let pollReturnDeferred: Deferred[]; +let server: Server; +let serverURL: URL; +let signal: AbortSignal; +let teardownServer: () => void; + +beforeEach(async () => { + pollCallDeferred = new Array(5).fill(undefined).map(() => createDeferred()); + pollReturnDeferred = new Array(5).fill(undefined).map(() => { + const deferred = createDeferred(); + + deferred.promise.catch(() => {}); + + return deferred; + }); + + jest.clearAllMocks(); + jest.useFakeTimers({ advanceTimers: true, now: 0 }); + + [serverURL, server, teardownServer] = await setup(); + + abortController = new AbortController(); +}); + +afterEach(() => { + teardownServer(); + + jest.useRealTimers(); + + return new Promise(resolve => { + signal.addEventListener('abort', () => resolve()); + + abortController.abort(); + + signal.aborted && resolve(); + }); +}); + +describe('with default options', () => { + beforeEach(() => { + signal = watchREST(serverURL, { signal: abortController.signal }); + }); + + describe('after first poll received', () => { + beforeEach(() => pollCallDeferred[0].promise); + + test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); + test('should not have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); + + describe('after 30 seconds', () => { + beforeEach(async () => { + jest.advanceTimersByTime(30_000); + + await pollCallDeferred[1].promise; + }); + + test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); + test('should have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); + + describe('after 30 seconds again', () => { + beforeEach(async () => { + jest.advanceTimersByTimeAsync(30_000); + + await pollCallDeferred[2].promise; + }); + + test('should have called API three times', () => waitFor(() => expect(pollFn).toBeCalledTimes(3))); + test('should have second API response', () => + expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(true)); + }); + }); + + describe('teardown server', () => { + beforeEach(() => teardownServer()); + + test('should abort the signal', () => waitFor(() => expect(signal.aborted).toBe(true))); + }); + + describe('when aborted', () => { + beforeEach(() => abortController.abort()); + + test('should signal', () => expect(signal.aborted).toBe(true)); + test('server should get the "close" event', () => expect(() => pollReturnDeferred[0].promise).rejects.toThrow('Socket closed.')); + }); + }); +}); + +describe('with pingInterval=45_000', () => { + beforeEach(() => { + signal = watchREST(serverURL, { + pingInterval: 45_000, + signal: abortController.signal + }); + }); + + describe('after first poll received', () => { + beforeEach(() => pollCallDeferred[0].promise); + + test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); + test('should not have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); + + describe('after 30 seconds', () => { + beforeEach(() => jest.advanceTimersByTime(30_000)); + + test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); + test('should have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); + + describe('after 30 seconds', () => { + beforeEach(() => jest.advanceTimersByTime(30_000)); + + test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); + test('should not have second API response', () => + expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(false)); + }); + }); + }); +}); + +describe('with pingInterval=15_000', () => { + beforeEach(() => { + signal = watchREST(serverURL, { + pingInterval: 15_000, + signal: abortController.signal + }); + }); + + describe('after first poll received', () => { + beforeEach(() => pollCallDeferred[0].promise); + + test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); + test('should not have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); + + describe('after 20 seconds', () => { + beforeEach(() => jest.advanceTimersByTime(20_000)); + + test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); + test('should not have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); + + describe('after 10 seconds', () => { + beforeEach(() => jest.advanceTimersByTime(10_000)); + + test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); + test('should have first API response', () => + expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); + test('should not have second API response', () => + expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(false)); + }); + }); + }); +}); diff --git a/src/streaming/watchREST.ts b/src/streaming/watchREST.ts new file mode 100644 index 00000000..c9115a23 --- /dev/null +++ b/src/streaming/watchREST.ts @@ -0,0 +1,93 @@ +import sleep from './sleep'; + +// The watchdog service should respond HTTP 2xx not sooner than 30 seconds. +// To prevent DoS the watchdog service, we should not send ping the watchdog service sooner than 25 seconds. +let DEFAULT_PING_INTERVAL = 25_000; +let MINIMUM_PING_INTERVAL = 10_000; + +type WatchRESTInit = { + /** + * Time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. + * + * The watching REST API endpoint should keep the polling call open for at least 30 seconds, then respond with HTTP 2xx. + * + * If the service return HTTP 2xx sooner than this value (25 seconds), we will wait until the time has passed before + * sending the ping again. In the meantime, the watchdog will not able to detect any connection issues. + * + * If the service return HTTP 2xx later than this value, we will wait until the service return HTTP 2xx. + */ + pingInterval?: number; + + /** + * Signal to abort the watchdog. + * + * When the signal is aborted, the watchdog signal will be aborted. + */ + signal?: AbortSignal; +}; + +/** + * Watches connectivity issues by continuously calling a long-polling REST API endpoint and returns an `AbortSignal` object. + * + * The returned `AbortSignal` object will be aborted when: + * + * 1. the REST API endpoint did not return a HTTP 2xx status, or; + * 2. the `WatchRESTInit.signal` is aborted. + * + * The REST API endpoint should keep the polling call open for 30 seconds, then respond with HTTP 2xx. + * + * Upon receiving HTTP 2xx, the watchdog will issue another long polling call immediately and not sooner than `pingInterval`. + * + * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends using a timeout value of 30 seconds. + */ +export default function watchREST(url: string | URL, { pingInterval, signal }: WatchRESTInit = {}): AbortSignal { + const abortController = new AbortController(); + + (async () => { + try { + // Rectifying `pingInterval`. + if (typeof pingInterval === 'number') { + pingInterval = Math.max(MINIMUM_PING_INTERVAL, pingInterval); + } else { + pingInterval = DEFAULT_PING_INTERVAL; + } + + // Rectifying `signal`. + if (signal && !(signal instanceof AbortSignal)) { + signal = undefined; + } + + while (!signal?.aborted) { + const pingAt = Date.now(); + + console.log(`WATCHDOG ${Date.now()}: Connecting to ${url}.`, signal); + + const res = await fetch(url, { signal }); + + if (!res.ok) { + console.log(`WATCHDOG ${Date.now()}: Failed to ping.`); + + throw new Error('Failed to ping REST endpoint.'); + } + + console.log(`WATCHDOG ${Date.now()}: Connected.`); + + await res.arrayBuffer(); + + console.log(`WATCHDOG ${Date.now()}: Got response.`, pingAt + pingInterval - Date.now()); + + // TODO: Warns if the API returns sooner than `pingInterval`. + + await sleep(pingAt + pingInterval - Date.now(), { signal }); + } + + console.log(`WATCHDOG ${Date.now()}: Aborted.`); + } catch (error) { + console.log(`WATCHDOG ${Date.now()}: Exception caught.`, error.message); + } finally { + abortController.abort(); + } + })(); + + return abortController.signal; +} From eed0d685bf787b1f9f6c707c4848973ec3e0c83b Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 11:45:46 -0700 Subject: [PATCH 02/33] Add warning if REST API returned too soon --- src/streaming/watchREST.test.ts | 16 +++++++++++++++- src/streaming/watchREST.ts | 28 +++++++++++++++------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/streaming/watchREST.test.ts b/src/streaming/watchREST.test.ts index 9882a48a..4c659f36 100644 --- a/src/streaming/watchREST.test.ts +++ b/src/streaming/watchREST.test.ts @@ -80,12 +80,15 @@ beforeEach(async () => { [serverURL, server, teardownServer] = await setup(); abortController = new AbortController(); + + jest.spyOn(console, 'warn').mockImplementation(() => {}); }); afterEach(() => { teardownServer(); jest.useRealTimers(); + console.warn['mockRestore']?.(); return new Promise(resolve => { signal.addEventListener('abort', () => resolve()); @@ -118,6 +121,7 @@ describe('with default options', () => { test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); test('should have first API response', () => expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); + test('should not warn', () => expect(console.warn).not.toBeCalled()); describe('after 30 seconds again', () => { beforeEach(async () => { @@ -129,6 +133,7 @@ describe('with default options', () => { test('should have called API three times', () => waitFor(() => expect(pollFn).toBeCalledTimes(3))); test('should have second API response', () => expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(true)); + test('should not warn', () => expect(console.warn).not.toBeCalled()); }); }); @@ -142,7 +147,8 @@ describe('with default options', () => { beforeEach(() => abortController.abort()); test('should signal', () => expect(signal.aborted).toBe(true)); - test('server should get the "close" event', () => expect(() => pollReturnDeferred[0].promise).rejects.toThrow('Socket closed.')); + test('server should get the "close" event', () => + expect(() => pollReturnDeferred[0].promise).rejects.toThrow('Socket closed.')); }); }); }); @@ -168,6 +174,13 @@ describe('with pingInterval=45_000', () => { test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); test('should have first API response', () => expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); + test('should warn once', () => + waitFor(() => + expect(console.warn).toHaveBeenNthCalledWith( + 1, + expect.stringContaining('REST API should not return sooner than the predefined `pingInterval` of 45000 ms.') + ) + )); describe('after 30 seconds', () => { beforeEach(() => jest.advanceTimersByTime(30_000)); @@ -210,6 +223,7 @@ describe('with pingInterval=15_000', () => { expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); test('should not have second API response', () => expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(false)); + test('should not warn', () => expect(console.warn).not.toBeCalled()); }); }); }); diff --git a/src/streaming/watchREST.ts b/src/streaming/watchREST.ts index c9115a23..24437c8d 100644 --- a/src/streaming/watchREST.ts +++ b/src/streaming/watchREST.ts @@ -44,6 +44,8 @@ export default function watchREST(url: string | URL, { pingInterval, signal }: W const abortController = new AbortController(); (async () => { + let warnedReturnTooSoon = false; + try { // Rectifying `pingInterval`. if (typeof pingInterval === 'number') { @@ -57,33 +59,33 @@ export default function watchREST(url: string | URL, { pingInterval, signal }: W signal = undefined; } + // Instead of listening to signal.addEventListener('abort'), we will just let fetch() handle all aborts. while (!signal?.aborted) { const pingAt = Date.now(); - - console.log(`WATCHDOG ${Date.now()}: Connecting to ${url}.`, signal); - const res = await fetch(url, { signal }); if (!res.ok) { - console.log(`WATCHDOG ${Date.now()}: Failed to ping.`); - throw new Error('Failed to ping REST endpoint.'); } - console.log(`WATCHDOG ${Date.now()}: Connected.`); - await res.arrayBuffer(); - console.log(`WATCHDOG ${Date.now()}: Got response.`, pingAt + pingInterval - Date.now()); - // TODO: Warns if the API returns sooner than `pingInterval`. + const timeToSleep = pingAt + pingInterval - Date.now(); - await sleep(pingAt + pingInterval - Date.now(), { signal }); - } + if (timeToSleep > 0) { + warnedReturnTooSoon || + console.warn( + `botframework-directlinejs: REST API should not return sooner than the predefined \`pingInterval\` of ${pingInterval} ms.` + ); - console.log(`WATCHDOG ${Date.now()}: Aborted.`); + warnedReturnTooSoon = true; + } + + await sleep(timeToSleep, { signal }); + } } catch (error) { - console.log(`WATCHDOG ${Date.now()}: Exception caught.`, error.message); + // If any error occurred, probably it is about connection issues. } finally { abortController.abort(); } From 3e1fd3f9498fcba8650bba35c5e02ba4e8067d8b Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 19:18:03 -0700 Subject: [PATCH 03/33] Add integration test --- .../__setup__/createBotProxy.ts | 47 ++++++- .../__setup__/expect/activityContaining.d.ts | 9 ++ .../__setup__/mockObserver.ts | 15 ++- .../__setup__/setupBotProxy.ts | 12 +- .../directLineStreaming/watchdog.story.ts | 121 ++++++++++++++++++ src/directLineStreaming.ts | 49 ++----- src/streaming/WebSocketClientWithWatchdog.ts | 1 + 7 files changed, 211 insertions(+), 43 deletions(-) create mode 100644 __tests__/directLineStreaming/__setup__/expect/activityContaining.d.ts create mode 100644 __tests__/directLineStreaming/watchdog.story.ts diff --git a/__tests__/directLineStreaming/__setup__/createBotProxy.ts b/__tests__/directLineStreaming/__setup__/createBotProxy.ts index fccf6f79..c7b9021f 100644 --- a/__tests__/directLineStreaming/__setup__/createBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/createBotProxy.ts @@ -29,9 +29,14 @@ type CreateBotProxyInit = { type CreateBotProxyReturnValue = { cleanUp: () => void; + closeAllWatchdogConnections: () => void; closeAllWebSocketConnections: () => void; directLineStreamingURL: string; directLineURL: string; + watchdogURL: string; + + get numWatchdogConnection(): number; + get numOverTheLifetimeWatchdogConnection(): number; }; const matchDirectLineStreamingProtocol = match('/.bot/', { decode: decodeURIComponent, end: false }); @@ -48,6 +53,8 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise void)[] = []; + let numOverTheLifetimeWatchdogConnection = 0; streamingBotURL && app.use('/.bot/', createProxyMiddleware({ changeOrigin: true, logLevel: 'silent', target: streamingBotURL })); @@ -95,6 +102,30 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { + const destroyResponse = () => { + res.destroy(); + }; + + closeAllWatchdogConnections.push(destroyResponse); + numOverTheLifetimeWatchdogConnection++; + + req.once('error', destroyResponse); + res.once('close', () => removeInline(closeAllWatchdogConnections, destroyResponse)); + + res.chunkedEncoding = true; + res.statusCode = 200; + + res.setHeader('cache-control', 'no-cache'); + res.setHeader('content-type', 'text/plain'); + res.write(' '); + + setTimeout(() => { + req.off('error', destroyResponse); + res.end(); + }, 30_000); + }); + const webSocketProxy = new WebSocketServer({ noServer: true }); webSocketProxy.on('connection', (socket: WebSocket, proxySocket: WebSocket, req: IncomingMessage) => { @@ -143,6 +174,7 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise {}); socket.once('close', () => proxySocket.close()); }) @@ -174,9 +206,22 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { + closeAllWatchdogConnections.forEach(close => close()); + closeAllWatchdogConnections.splice(0); + }, closeAllWebSocketConnections, directLineStreamingURL: new URL('/.bot/v3/directline', url).href, - directLineURL: new URL('/v3/directline', url).href + directLineURL: new URL('/v3/directline', url).href, + watchdogURL: new URL('/test/watchdog', url).href, + + get numWatchdogConnection(): number { + return closeAllWatchdogConnections.length; + }, + + get numOverTheLifetimeWatchdogConnection(): number { + return numOverTheLifetimeWatchdogConnection; + } }); }); } catch (error) { diff --git a/__tests__/directLineStreaming/__setup__/expect/activityContaining.d.ts b/__tests__/directLineStreaming/__setup__/expect/activityContaining.d.ts new file mode 100644 index 00000000..1e7e9e16 --- /dev/null +++ b/__tests__/directLineStreaming/__setup__/expect/activityContaining.d.ts @@ -0,0 +1,9 @@ +import '@jest/types'; + +declare global { + namespace jest { + interface Expect { + activityContaining(messageText: string, mergeActivity?: { id?: string; type?: string }): any; + } + } +} diff --git a/__tests__/directLineStreaming/__setup__/mockObserver.ts b/__tests__/directLineStreaming/__setup__/mockObserver.ts index 781911f9..690eff89 100644 --- a/__tests__/directLineStreaming/__setup__/mockObserver.ts +++ b/__tests__/directLineStreaming/__setup__/mockObserver.ts @@ -12,19 +12,24 @@ type Observation = * Mocks an observer and records all observations. */ export default function mockObserver(): Readonly< - Required> & { observations: ReadonlyArray> } + Required> & { + observations: ReadonlyArray>; + observe: (observation: Observation) => void; + } > { + const observe: (observation: Observation) => void = jest.fn(observation => observations.push(observation)); const observations: Array> = []; - const complete = jest.fn(() => observations.push([Date.now(), 'complete'])); - const error = jest.fn(reason => observations.push([Date.now(), 'error', reason])); - const next = jest.fn(value => observations.push([Date.now(), 'next', value])); - const start = jest.fn(subscription => observations.push([Date.now(), 'start', subscription])); + const complete = jest.fn(() => observe([Date.now(), 'complete'])); + const error = jest.fn(reason => observe([Date.now(), 'error', reason])); + const next = jest.fn(value => observe([Date.now(), 'next', value])); + const start = jest.fn(subscription => observe([Date.now(), 'start', subscription])); return Object.freeze({ complete, error, next, + observe, start, get observations() { diff --git a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts index aa7ccfee..a4c066fa 100644 --- a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts @@ -19,9 +19,19 @@ export default async function setupBotProxy( botProxies.push(botProxy); return { + closeAllWatchdogConnections: botProxy.closeAllWatchdogConnections, closeAllWebSocketConnections: botProxy.closeAllWebSocketConnections, directLineURL: botProxy.directLineURL, - directLineStreamingURL: botProxy.directLineStreamingURL + directLineStreamingURL: botProxy.directLineStreamingURL, + watchdogURL: botProxy.watchdogURL, + + get numWatchdogConnection() { + return botProxy.numWatchdogConnection; + }, + + get numOverTheLifetimeWatchdogConnection() { + return botProxy.numOverTheLifetimeWatchdogConnection; + } }; } diff --git a/__tests__/directLineStreaming/watchdog.story.ts b/__tests__/directLineStreaming/watchdog.story.ts new file mode 100644 index 00000000..c2f9affe --- /dev/null +++ b/__tests__/directLineStreaming/watchdog.story.ts @@ -0,0 +1,121 @@ +/// + +import fetch from 'node-fetch'; + +import { ConnectionStatus } from '../../src/directLine'; +import { DirectLineStreaming } from '../../src/directLineStreaming'; +import mockObserver from './__setup__/mockObserver'; +import setupBotProxy from './__setup__/setupBotProxy'; +import waitFor from './__setup__/external/testing-library/waitFor'; + +type MockObserver = ReturnType; + +const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; +const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; + +type ResultOfPromise = T extends PromiseLike ? P : never; + +jest.setTimeout(10_000); + +// GIVEN: A Direct Line Streaming chat adapter. +describe('Direct Line Streaming chat adapter with watchdog', () => { + let activityObserver: MockObserver; + let botProxy: ResultOfPromise>; + let connectionStatusObserver: MockObserver; + let directLine: DirectLineStreaming; + + beforeEach(async () => { + jest.useFakeTimers({ now: 0 }); + + let token: string; + + [botProxy, { token }] = await Promise.all([ + setupBotProxy({ streamingBotURL: MOCKBOT3_URL }), + fetch(TOKEN_URL, { method: 'POST' }).then(res => res.json()) + ]); + + activityObserver = mockObserver(); + connectionStatusObserver = mockObserver(); + directLine = new DirectLineStreaming({ + domain: botProxy.directLineStreamingURL, + token, + watchdog: { url: botProxy.watchdogURL } + }); + + directLine.connectionStatus$.subscribe(connectionStatusObserver); + }); + + afterEach(() => { + directLine.end(); + + jest.useRealTimers(); + }); + + describe('when connect', () => { + // WHEN: Connect. + beforeEach(() => directLine.activity$.subscribe(activityObserver)); + + // THEN: Should observe "Uninitialized" -> "Connecting" -> "Online". + test('should observe "Uninitialized" -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + + // WHEN: Connection status become "Online" and watchdog is connected. + describe('after online and watchdog is connected', () => { + beforeEach(() => + waitFor( + () => { + expect(connectionStatusObserver.observe).toHaveBeenLastCalledWith([ + expect.any(Number), + 'next', + ConnectionStatus.Online + ]); + + expect(botProxy.numWatchdogConnection).toBe(1); + expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(1); + }, + { timeout: 5_000 } + ) + ); + + // THEN: Should receive "Hello and welcome!" + test('should receive "Hello and welcome!"', () => + waitFor( + () => + expect(activityObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', expect.activityContaining('Hello and welcome!')] + ]), + { timeout: 5_000 } + )); + + // WHEN: Watchdog connection is closed. + describe('when watchdog failed', () => { + beforeEach(() => botProxy.closeAllWatchdogConnections()); + + // THEN: Should observe "Connecting" -> "Online" again. + test('should observe ... -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + + test('should reconnect watchdog', () => + waitFor(() => expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(2), { timeout: 5_000 })); + }); + }); + }); +}); diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index e6f8fb8f..bd22e988 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -141,33 +141,28 @@ export class DirectLineStreaming implements IBotConnection { private _botAgent = ''; - #watchdog: - | ((init: { signal: AbortSignal }) => AbortSignal) - | { - pingInterval?: number; - url: string | URL; - } - | undefined; + #watchdog: ((init: { signal: AbortSignal }) => AbortSignal) | undefined; constructor(options: DirectLineStreamingOptions) { - // Verifies options.watchdog. + // Rectifies options.watchdog. const watchdog = options?.watchdog; - if ( - !( - typeof watchdog === 'undefined' || - typeof watchdog === 'function' || - ((typeof watchdog.pingInterval === 'number' || typeof watchdog.pingInterval === 'undefined') && - (typeof watchdog.url === 'string' || watchdog.url instanceof URL)) - ) + if (typeof watchdog === 'function') { + this.#watchdog = watchdog; + } else if (typeof watchdog === 'undefined') { + // Intentionally left blank. + } else if ( + (typeof watchdog.pingInterval === 'number' || typeof watchdog.pingInterval === 'undefined') && + (typeof watchdog.url === 'string' || watchdog.url instanceof URL) ) { + this.#watchdog = ({ signal }: { signal: AbortSignal }) => + watchREST(watchdog.url, { pingInterval: watchdog.pingInterval, signal }); + } else { throw new Error( 'botframework-directlinejs: "watchdog" option must be either a function returning an AbortSignal, an object, or undefined.' ); } - this.#watchdog = options?.watchdog; - this.token = options.token; this.refreshToken().catch(() => { @@ -196,8 +191,6 @@ export class DirectLineStreaming implements IBotConnection { } public reconnect({ conversationId, token }: Conversation) { - console.log('DLASE: reconnect', conversationId); - if (this.connectionStatus$.getValue() === ConnectionStatus.Ended) { throw new Error('Connection has ended.'); } @@ -393,14 +386,7 @@ export class DirectLineStreaming implements IBotConnection { return new Promise(async (resolve, reject) => { try { const watchdog: AbortSignal | undefined = - typeof this.#watchdog === 'function' - ? this.#watchdog({ signal: abortController.signal }) - : typeof this.#watchdog === 'undefined' - ? undefined - : watchREST(this.#watchdog.url, { - pingInterval: this.#watchdog.pingInterval, - signal: abortController.signal - }); + typeof this.#watchdog === 'function' ? this.#watchdog({ signal: abortController.signal }) : undefined; this.streamConnection = new WebSocketClientWithWatchdog({ disconnectionHandler: resolve, @@ -471,20 +457,14 @@ export class DirectLineStreaming implements IBotConnection { const start = Date.now(); try { - console.log('DLASE: connectAsync', numRetries); - // This promise will reject/resolve when disconnected. await this.connectAsync(); } catch (err) { console.error(err); } - console.log('DLASE: connection broken'); - // If someone call end() to break the connection, we will never listen to any reconnect(). if (this.connectionStatus$.getValue() === ConnectionStatus.Ended) { - console.log('DLASE: ended'); - // This is the only place the loop in this function will be broke. return; } @@ -499,16 +479,13 @@ export class DirectLineStreaming implements IBotConnection { // - we should reset the retry counter, and; // - we should reconnect immediately. if (60000 < Date.now() - start) { - console.log('DLASE: good connection, reset retry and no delay'); numRetries = MAX_RETRY_COUNT; } else if (numRetries > 0) { // Sleep only if we are doing retry. Otherwise, we are going to break the loop and signal FailedToConnect. - console.log('DLASE: poor connection, delay'); await new Promise(r => setTimeout(r, this.getRetryDelay())); } } - console.log('DLASE: failed to connect'); // TODO: [TEST] Make sure FailedToConnect is reported immediately after last disconnection, should be no getRetryDelay(). // Failed to reconnect after multiple retries. this.connectionStatus$.next(ConnectionStatus.FailedToConnect); diff --git a/src/streaming/WebSocketClientWithWatchdog.ts b/src/streaming/WebSocketClientWithWatchdog.ts index a46ffc7d..a601deb8 100644 --- a/src/streaming/WebSocketClientWithWatchdog.ts +++ b/src/streaming/WebSocketClientWithWatchdog.ts @@ -31,6 +31,7 @@ export default class WebSocketClientWithWatchdog extends WebSocketClient { // TODO: Better, the `watchdog` should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. // `BrowserWebSocket` is where it creates `WebSocket` object. // The `watchdog` object should accompany `WebSocket` and forcibly close it on abort. + // Maybe `botframework-streaming` should accept ponyfills. connect(): Promise { if (this.#connectCalled) { console.warn('botframework-directlinejs: connect() can only be called once.'); From e02817bab61ca2ab73b91f5c3361fc097fb80635 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 20:02:07 -0700 Subject: [PATCH 04/33] Add NetworkInformation watchdog --- .../directLineStreaming/watchdog.story.ts | 3 +- src/streaming/watchNetworkInformation.test.ts | 31 ++++++++++++ src/streaming/watchNetworkInformation.ts | 49 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 src/streaming/watchNetworkInformation.test.ts create mode 100644 src/streaming/watchNetworkInformation.ts diff --git a/__tests__/directLineStreaming/watchdog.story.ts b/__tests__/directLineStreaming/watchdog.story.ts index c2f9affe..9a50959f 100644 --- a/__tests__/directLineStreaming/watchdog.story.ts +++ b/__tests__/directLineStreaming/watchdog.story.ts @@ -9,12 +9,11 @@ import setupBotProxy from './__setup__/setupBotProxy'; import waitFor from './__setup__/external/testing-library/waitFor'; type MockObserver = ReturnType; +type ResultOfPromise = T extends PromiseLike ? P : never; const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; -type ResultOfPromise = T extends PromiseLike ? P : never; - jest.setTimeout(10_000); // GIVEN: A Direct Line Streaming chat adapter. diff --git a/src/streaming/watchNetworkInformation.test.ts b/src/streaming/watchNetworkInformation.test.ts new file mode 100644 index 00000000..ac777e86 --- /dev/null +++ b/src/streaming/watchNetworkInformation.test.ts @@ -0,0 +1,31 @@ +import watchNetworkInformation from './watchNetworkInformation'; + +let abortController: AbortController; + +beforeEach(() => { + const networkInformation = new EventTarget(); + + (global as any).navigator = { + get connection() { + return networkInformation; + } + }; + + abortController = new AbortController(); +}); + +describe('after constructed', () => { + let watchdog: AbortSignal; + + beforeEach(() => { + watchdog = watchNetworkInformation({ signal: abortController.signal }); + }); + + test('should not be aborted', () => expect(watchdog.aborted).toBe(false)); + + describe('when "change" event is received', () => { + beforeEach(() => navigator.connection.dispatchEvent(new Event('change'))); + + test('should be aborted', () => expect(watchdog.aborted).toBe(true)); + }); +}); diff --git a/src/streaming/watchNetworkInformation.ts b/src/streaming/watchNetworkInformation.ts new file mode 100644 index 00000000..9796859f --- /dev/null +++ b/src/streaming/watchNetworkInformation.ts @@ -0,0 +1,49 @@ +declare global { + // This is subset of https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation. + interface NetworkInformation extends EventTarget { + addEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + + removeEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + } + + interface Navigator { + get connection(): NetworkInformation; + } +} + +type WatchNetworkInformationInit = { + signal: AbortSignal; +}; + +/** + * Watches the connection a device is using to communicate with the network. + * + * When the connection change, watchdog will fail. + */ +export default function watchNetworkInformation({ signal }: WatchNetworkInformationInit): AbortSignal { + const abortController = new AbortController(); + const handleNetworkInformationChange = () => abortController.abort(); + + const connection = navigator?.connection; + + if (connection) { + connection.addEventListener('change', handleNetworkInformationChange, { once: true }); + + signal?.addEventListener('abort', () => { + connection.removeEventListener('change', handleNetworkInformationChange); + abortController.abort(); + }); + } else { + console.warn('botframework-directlinejs: NetworkInformation API is not supported in the current environment.'); + } + + return abortController.signal; +} From 3b8a65cbd92ac7e417dcd771aad24e23535a2372 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 20:27:40 -0700 Subject: [PATCH 05/33] Add network information watchdog --- .../watchdog.networkInformation.story.ts | 121 ++++++++++++++++++ ...tchdog.story.ts => watchdog.rest.story.ts} | 4 +- src/directLineStreaming.ts | 17 ++- src/streaming/watchNetworkInformation.ts | 6 +- 4 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 __tests__/directLineStreaming/watchdog.networkInformation.story.ts rename __tests__/directLineStreaming/{watchdog.story.ts => watchdog.rest.story.ts} (96%) diff --git a/__tests__/directLineStreaming/watchdog.networkInformation.story.ts b/__tests__/directLineStreaming/watchdog.networkInformation.story.ts new file mode 100644 index 00000000..835252c7 --- /dev/null +++ b/__tests__/directLineStreaming/watchdog.networkInformation.story.ts @@ -0,0 +1,121 @@ +/// + +import fetch from 'node-fetch'; + +import { ConnectionStatus } from '../../src/directLine'; +import { DirectLineStreaming } from '../../src/directLineStreaming'; +import mockObserver from './__setup__/mockObserver'; +import setupBotProxy from './__setup__/setupBotProxy'; +import waitFor from './__setup__/external/testing-library/waitFor'; + +type MockObserver = ReturnType; +type ResultOfPromise = T extends PromiseLike ? P : never; + +const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; +const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; + +jest.setTimeout(10_000); + +// GIVEN: A Direct Line Streaming chat adapter with watchdog on Network Information API. +describe('Direct Line Streaming chat adapter with watchdog on Network Information API', () => { + let activityObserver: MockObserver; + let botProxy: ResultOfPromise>; + let connectionStatusObserver: MockObserver; + let directLine: DirectLineStreaming; + + beforeEach(async () => { + jest.useFakeTimers({ now: 0 }); + + const networkInformation = new EventTarget(); + + (global as any).navigator = { + get connection() { + return networkInformation; + } + }; + + let token: string; + + [botProxy, { token }] = await Promise.all([ + setupBotProxy({ streamingBotURL: MOCKBOT3_URL }), + fetch(TOKEN_URL, { method: 'POST' }).then(res => res.json()) + ]); + + activityObserver = mockObserver(); + connectionStatusObserver = mockObserver(); + directLine = new DirectLineStreaming({ + domain: botProxy.directLineStreamingURL, + token, + watchdog: 'network information' + }); + + directLine.connectionStatus$.subscribe(connectionStatusObserver); + }); + + afterEach(() => { + directLine.end(); + + jest.useRealTimers(); + }); + + describe('when connect', () => { + // WHEN: Connect. + beforeEach(() => directLine.activity$.subscribe(activityObserver)); + + // THEN: Should observe "Uninitialized" -> "Connecting" -> "Online". + test('should observe "Uninitialized" -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + + // WHEN: Connection status become "Online". + describe('after online', () => { + beforeEach(() => + waitFor( + () => + expect(connectionStatusObserver.observe).toHaveBeenLastCalledWith([ + expect.any(Number), + 'next', + ConnectionStatus.Online + ]), + { timeout: 5_000 } + ) + ); + + // THEN: Should receive "Hello and welcome!" + test('should receive "Hello and welcome!"', () => + waitFor( + () => + expect(activityObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', expect.activityContaining('Hello and welcome!')] + ]), + { timeout: 5_000 } + )); + + // WHEN: "change" event is received. + describe('when "change" event is received', () => { + beforeEach(() => navigator.connection.dispatchEvent(new Event('change'))); + + // THEN: Should observe "Connecting" -> "Online" again. + test('should observe ... -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + }); + }); + }); +}); diff --git a/__tests__/directLineStreaming/watchdog.story.ts b/__tests__/directLineStreaming/watchdog.rest.story.ts similarity index 96% rename from __tests__/directLineStreaming/watchdog.story.ts rename to __tests__/directLineStreaming/watchdog.rest.story.ts index 9a50959f..94f3d4db 100644 --- a/__tests__/directLineStreaming/watchdog.story.ts +++ b/__tests__/directLineStreaming/watchdog.rest.story.ts @@ -16,8 +16,8 @@ const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directli jest.setTimeout(10_000); -// GIVEN: A Direct Line Streaming chat adapter. -describe('Direct Line Streaming chat adapter with watchdog', () => { +// GIVEN: A Direct Line Streaming chat adapter with watchdog on REST API. +describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { let activityObserver: MockObserver; let botProxy: ResultOfPromise>; let connectionStatusObserver: MockObserver; diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index bd22e988..88ba74e5 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -8,6 +8,7 @@ import * as BFSE from 'botframework-streaming'; import createDeferred from './createDeferred'; import fetch from 'cross-fetch'; +import watchNetworkInformation from './streaming/watchNetworkInformation'; import watchREST from './streaming/watchREST'; import WebSocketClientWithWatchdog from './streaming/WebSocketClientWithWatchdog'; @@ -36,12 +37,13 @@ interface DirectLineStreamingOptions { * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` did not emit `error` event timely in case of connection issues. * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. * - * There are 2 ways to set the probe: `object` or `function`. + * There are 3 ways to set the probe: `object` (REST API watchdog), `function`, or `"network information"` (via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)). * - * When the probe is an object: + * When the probe is an object, it will watch the liveness of a long-polling REST API: * * - `url` is the URL of a REST long-polling API. The REST API must keep the connection for a period of time and returns HTTP 2xx when it end. * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. + * - If non-2XX status code is received, it will be treated as a fault. * - `pingInterval` is the time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. * It must be shorter than the time the API would keep the connection. * @@ -52,13 +54,20 @@ interface DirectLineStreamingOptions { * - The function should create a new probe on every call and probe should not be reused. * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. * - At any point of time, there should be no more than 1 probe active. The chat adapter is expected to signal the release of probe before requesting for a new one. + * + * When the probe is `"network information"`: + * + * - It will use [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). + * - If the environment does not support Network Information API, it will be treated as if no probe is defined. + * - When `change` event is received, the watchdog will treat it as a fault. */ watchdog?: | ((init: { signal: AbortSignal }) => AbortSignal) | { pingInterval?: number; url: string | URL; - }; + } + | 'network information'; } class StreamHandler implements BFSE.RequestHandler { @@ -149,6 +158,8 @@ export class DirectLineStreaming implements IBotConnection { if (typeof watchdog === 'function') { this.#watchdog = watchdog; + } else if (watchdog === 'network information') { + this.#watchdog = watchNetworkInformation; } else if (typeof watchdog === 'undefined') { // Intentionally left blank. } else if ( diff --git a/src/streaming/watchNetworkInformation.ts b/src/streaming/watchNetworkInformation.ts index 9796859f..8ff86416 100644 --- a/src/streaming/watchNetworkInformation.ts +++ b/src/streaming/watchNetworkInformation.ts @@ -24,9 +24,9 @@ type WatchNetworkInformationInit = { }; /** - * Watches the connection a device is using to communicate with the network. + * Watches the connection a device is using to communicate with the network via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). * - * When the connection change, watchdog will fail. + * When the connection change, watchdog will treat it as a fault. */ export default function watchNetworkInformation({ signal }: WatchNetworkInformationInit): AbortSignal { const abortController = new AbortController(); @@ -42,7 +42,7 @@ export default function watchNetworkInformation({ signal }: WatchNetworkInformat abortController.abort(); }); } else { - console.warn('botframework-directlinejs: NetworkInformation API is not supported in the current environment.'); + console.warn('botframework-directlinejs: Network Information API is not supported in the current environment, liveness probe will not report fault.'); } return abortController.signal; From 74945c13bb48b7dffb428131ec52b58f249c5bf4 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 21:19:44 -0700 Subject: [PATCH 06/33] Prolong test time --- __tests__/directLineStreaming/retryConnect.fail.story.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/__tests__/directLineStreaming/retryConnect.fail.story.js b/__tests__/directLineStreaming/retryConnect.fail.story.js index 9e85746f..63e1cbb9 100644 --- a/__tests__/directLineStreaming/retryConnect.fail.story.js +++ b/__tests__/directLineStreaming/retryConnect.fail.story.js @@ -9,6 +9,8 @@ import waitFor from './__setup__/external/testing-library/waitFor'; const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; +jest.setTimeout(15000); + afterEach(() => jest.useRealTimers()); test('reconnect fail should stop', async () => { @@ -72,7 +74,7 @@ test('reconnect fail should stop', async () => { closeAllWebSocketConnections(); // THEN: Server should observe three Web Socket connections. - await waitFor(() => expect(onUpgrade).toBeCalledTimes(3)); + await waitFor(() => expect(onUpgrade).toBeCalledTimes(3), { timeout: 5000 }); // THEN: Should not wait before reconnecting the first time. // This is because the connection has been established for more than 1 minute and is considered stable. @@ -108,7 +110,7 @@ test('reconnect fail should stop', async () => { }); // THEN: Server should observe 3 connections again. - await waitFor(() => expect(onUpgrade).toBeCalledTimes(6)); + await waitFor(() => expect(onUpgrade).toBeCalledTimes(6), { timeout: 5000 }); // THEN: Should not wait before reconnecting. // This is because calling reconnect() should not by delayed. From 1bf6c21649d5f4c8bfaf0d1aaa0d6cbc29561ef4 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 21:21:22 -0700 Subject: [PATCH 07/33] Support NetworkInformation-like --- .../watchdog.networkInformation.story.ts | 2 +- src/directLineStreaming.ts | 37 ++++++++++--------- src/streaming/watchNetworkInformation.test.ts | 12 ++---- src/streaming/watchNetworkInformation.ts | 21 +++++------ src/streaming/watchREST.test.ts | 10 ++--- src/streaming/watchREST.ts | 37 ++++++++++--------- 6 files changed, 58 insertions(+), 61 deletions(-) diff --git a/__tests__/directLineStreaming/watchdog.networkInformation.story.ts b/__tests__/directLineStreaming/watchdog.networkInformation.story.ts index 835252c7..d7e59061 100644 --- a/__tests__/directLineStreaming/watchdog.networkInformation.story.ts +++ b/__tests__/directLineStreaming/watchdog.networkInformation.story.ts @@ -46,7 +46,7 @@ describe('Direct Line Streaming chat adapter with watchdog on Network Informatio directLine = new DirectLineStreaming({ domain: botProxy.directLineStreamingURL, token, - watchdog: 'network information' + watchdog: navigator.connection }); directLine.connectionStatus$.subscribe(connectionStatusObserver); diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index 88ba74e5..b88f0ce4 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -37,15 +37,19 @@ interface DirectLineStreamingOptions { * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` did not emit `error` event timely in case of connection issues. * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. * - * There are 3 ways to set the probe: `object` (REST API watchdog), `function`, or `"network information"` (via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)). + * There are 3 ways to set the probe: `NetworkInformation` (via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API watchdog), or `function`. + * + * When the probe is an instance of `NetworkInformation`: + * + * - When `change` event is received on the instance, the watchdog will treat it as a fault. * * When the probe is an object, it will watch the liveness of a long-polling REST API: * * - `url` is the URL of a REST long-polling API. The REST API must keep the connection for a period of time and returns HTTP 2xx when it end. * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. - * - If non-2XX status code is received, it will be treated as a fault. - * - `pingInterval` is the time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. - * It must be shorter than the time the API would keep the connection. + * - If a non-2XX status code is received, it will be treated as a fault. + * - `minimumInterval` is the time to wait between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. + * It should be shorter than the time the API would keep the connection. * * When the probe is a function: * @@ -54,20 +58,14 @@ interface DirectLineStreamingOptions { * - The function should create a new probe on every call and probe should not be reused. * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. * - At any point of time, there should be no more than 1 probe active. The chat adapter is expected to signal the release of probe before requesting for a new one. - * - * When the probe is `"network information"`: - * - * - It will use [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). - * - If the environment does not support Network Information API, it will be treated as if no probe is defined. - * - When `change` event is received, the watchdog will treat it as a fault. */ watchdog?: - | ((init: { signal: AbortSignal }) => AbortSignal) + | NetworkInformation | { - pingInterval?: number; + minimumInterval?: number; url: string | URL; } - | 'network information'; + | ((init: { signal: AbortSignal }) => AbortSignal); } class StreamHandler implements BFSE.RequestHandler { @@ -158,16 +156,21 @@ export class DirectLineStreaming implements IBotConnection { if (typeof watchdog === 'function') { this.#watchdog = watchdog; - } else if (watchdog === 'network information') { - this.#watchdog = watchNetworkInformation; } else if (typeof watchdog === 'undefined') { // Intentionally left blank. } else if ( - (typeof watchdog.pingInterval === 'number' || typeof watchdog.pingInterval === 'undefined') && + watchdog instanceof EventTarget || + // We also accept `EventTargetLike`. + (typeof watchdog['addEventListener'] === 'function' && typeof watchdog['removeEventListener'] === 'function') + ) { + this.#watchdog = ({ signal }: { signal: AbortSignal }) => + watchNetworkInformation(watchdog as NetworkInformation, { signal }); + } else if ( + (typeof watchdog.minimumInterval === 'number' || typeof watchdog.minimumInterval === 'undefined') && (typeof watchdog.url === 'string' || watchdog.url instanceof URL) ) { this.#watchdog = ({ signal }: { signal: AbortSignal }) => - watchREST(watchdog.url, { pingInterval: watchdog.pingInterval, signal }); + watchREST(watchdog.url, { minimumInterval: watchdog.minimumInterval, signal }); } else { throw new Error( 'botframework-directlinejs: "watchdog" option must be either a function returning an AbortSignal, an object, or undefined.' diff --git a/src/streaming/watchNetworkInformation.test.ts b/src/streaming/watchNetworkInformation.test.ts index ac777e86..b95cd3ff 100644 --- a/src/streaming/watchNetworkInformation.test.ts +++ b/src/streaming/watchNetworkInformation.test.ts @@ -1,24 +1,18 @@ import watchNetworkInformation from './watchNetworkInformation'; let abortController: AbortController; +let networkInformation: EventTarget; beforeEach(() => { - const networkInformation = new EventTarget(); - - (global as any).navigator = { - get connection() { - return networkInformation; - } - }; - abortController = new AbortController(); + networkInformation = new EventTarget(); }); describe('after constructed', () => { let watchdog: AbortSignal; beforeEach(() => { - watchdog = watchNetworkInformation({ signal: abortController.signal }); + watchdog = watchNetworkInformation(networkInformation, { signal: abortController.signal }); }); test('should not be aborted', () => expect(watchdog.aborted).toBe(false)); diff --git a/src/streaming/watchNetworkInformation.ts b/src/streaming/watchNetworkInformation.ts index 8ff86416..63021a56 100644 --- a/src/streaming/watchNetworkInformation.ts +++ b/src/streaming/watchNetworkInformation.ts @@ -28,22 +28,19 @@ type WatchNetworkInformationInit = { * * When the connection change, watchdog will treat it as a fault. */ -export default function watchNetworkInformation({ signal }: WatchNetworkInformationInit): AbortSignal { +export default function watchNetworkInformation( + connection: NetworkInformation, + { signal }: WatchNetworkInformationInit +): AbortSignal { const abortController = new AbortController(); const handleNetworkInformationChange = () => abortController.abort(); - const connection = navigator?.connection; + connection.addEventListener('change', handleNetworkInformationChange, { once: true }); - if (connection) { - connection.addEventListener('change', handleNetworkInformationChange, { once: true }); - - signal?.addEventListener('abort', () => { - connection.removeEventListener('change', handleNetworkInformationChange); - abortController.abort(); - }); - } else { - console.warn('botframework-directlinejs: Network Information API is not supported in the current environment, liveness probe will not report fault.'); - } + signal?.addEventListener('abort', () => { + connection.removeEventListener('change', handleNetworkInformationChange); + abortController.abort(); + }); return abortController.signal; } diff --git a/src/streaming/watchREST.test.ts b/src/streaming/watchREST.test.ts index 4c659f36..55db6970 100644 --- a/src/streaming/watchREST.test.ts +++ b/src/streaming/watchREST.test.ts @@ -153,10 +153,10 @@ describe('with default options', () => { }); }); -describe('with pingInterval=45_000', () => { +describe('with minimumInterval=45_000', () => { beforeEach(() => { signal = watchREST(serverURL, { - pingInterval: 45_000, + minimumInterval: 45_000, signal: abortController.signal }); }); @@ -178,7 +178,7 @@ describe('with pingInterval=45_000', () => { waitFor(() => expect(console.warn).toHaveBeenNthCalledWith( 1, - expect.stringContaining('REST API should not return sooner than the predefined `pingInterval` of 45000 ms.') + expect.stringContaining('REST API should not return sooner than the predefined `minimumInterval` of 45000 ms.') ) )); @@ -193,10 +193,10 @@ describe('with pingInterval=45_000', () => { }); }); -describe('with pingInterval=15_000', () => { +describe('with minimumInterval=15_000', () => { beforeEach(() => { signal = watchREST(serverURL, { - pingInterval: 15_000, + minimumInterval: 15_000, signal: abortController.signal }); }); diff --git a/src/streaming/watchREST.ts b/src/streaming/watchREST.ts index 24437c8d..ee6905dc 100644 --- a/src/streaming/watchREST.ts +++ b/src/streaming/watchREST.ts @@ -2,26 +2,29 @@ import sleep from './sleep'; // The watchdog service should respond HTTP 2xx not sooner than 30 seconds. // To prevent DoS the watchdog service, we should not send ping the watchdog service sooner than 25 seconds. -let DEFAULT_PING_INTERVAL = 25_000; -let MINIMUM_PING_INTERVAL = 10_000; +let DEFAULT_MINIMUM_INTERVAL = 25_000; +let MINIMUM_MINIMUM_INTERVAL = 10_000; type WatchRESTInit = { /** - * Time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. + * Minimum time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. * * The watching REST API endpoint should keep the polling call open for at least 30 seconds, then respond with HTTP 2xx. * - * If the service return HTTP 2xx sooner than this value (25 seconds), we will wait until the time has passed before - * sending the ping again. In the meantime, the watchdog will not able to detect any connection issues. + * If the service respond with HTTP 2xx sooner than this interval, the next call will be delayed until the interval has passed. + * In the meantime, the watchdog will not be able to detect any connection faults. * - * If the service return HTTP 2xx later than this value, we will wait until the service return HTTP 2xx. + * If the service respond with HTTP 2xx on or later than this interval, the next call will be made immediately. + * + * For example, assumes the interval is set to 25 seconds. If the service responded after 10 seconds the call is being made, + * the watchdog will wait for 15 seconds before making another call. */ - pingInterval?: number; + minimumInterval?: number; /** * Signal to abort the watchdog. * - * When the signal is aborted, the watchdog signal will be aborted. + * When this signal is being aborted, the watchdog will treat it as a fault. */ signal?: AbortSignal; }; @@ -36,22 +39,22 @@ type WatchRESTInit = { * * The REST API endpoint should keep the polling call open for 30 seconds, then respond with HTTP 2xx. * - * Upon receiving HTTP 2xx, the watchdog will issue another long polling call immediately and not sooner than `pingInterval`. + * Upon receiving HTTP 2xx, the watchdog will issue another long polling call immediately and not sooner than `minimumInterval`. * * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends using a timeout value of 30 seconds. */ -export default function watchREST(url: string | URL, { pingInterval, signal }: WatchRESTInit = {}): AbortSignal { +export default function watchREST(url: string | URL, { minimumInterval, signal }: WatchRESTInit = {}): AbortSignal { const abortController = new AbortController(); (async () => { let warnedReturnTooSoon = false; try { - // Rectifying `pingInterval`. - if (typeof pingInterval === 'number') { - pingInterval = Math.max(MINIMUM_PING_INTERVAL, pingInterval); + // Rectifying `minimumInterval`. + if (typeof minimumInterval === 'number') { + minimumInterval = Math.max(MINIMUM_MINIMUM_INTERVAL, minimumInterval); } else { - pingInterval = DEFAULT_PING_INTERVAL; + minimumInterval = DEFAULT_MINIMUM_INTERVAL; } // Rectifying `signal`. @@ -70,13 +73,13 @@ export default function watchREST(url: string | URL, { pingInterval, signal }: W await res.arrayBuffer(); - // TODO: Warns if the API returns sooner than `pingInterval`. - const timeToSleep = pingAt + pingInterval - Date.now(); + // TODO: Warns if the API returns sooner than `minimumInterval`. + const timeToSleep = pingAt + minimumInterval - Date.now(); if (timeToSleep > 0) { warnedReturnTooSoon || console.warn( - `botframework-directlinejs: REST API should not return sooner than the predefined \`pingInterval\` of ${pingInterval} ms.` + `botframework-directlinejs: REST API should not return sooner than the predefined \`minimumInterval\` of ${minimumInterval} ms.` ); warnedReturnTooSoon = true; From dc2eda4b5ccf7725537fee64482837351286b565 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 21:48:09 -0700 Subject: [PATCH 08/33] Add AbortSignal function story --- .../watchdog.function.story.ts | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 __tests__/directLineStreaming/watchdog.function.story.ts diff --git a/__tests__/directLineStreaming/watchdog.function.story.ts b/__tests__/directLineStreaming/watchdog.function.story.ts new file mode 100644 index 00000000..0ac51280 --- /dev/null +++ b/__tests__/directLineStreaming/watchdog.function.story.ts @@ -0,0 +1,147 @@ +/// + +import fetch from 'node-fetch'; + +import { ConnectionStatus } from '../../src/directLine'; +import { DirectLineStreaming } from '../../src/directLineStreaming'; +import mockObserver from './__setup__/mockObserver'; +import setupBotProxy from './__setup__/setupBotProxy'; +import waitFor from './__setup__/external/testing-library/waitFor'; + +type MockObserver = ReturnType; +type ResultOfPromise = T extends PromiseLike ? P : never; + +const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; +const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; + +jest.setTimeout(10_000); + +// GIVEN: A Direct Line Streaming chat adapter with watchdog on REST API. +describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { + let activityObserver: MockObserver; + let botProxy: ResultOfPromise>; + let connectionStatusObserver: MockObserver; + let createAbortController: jest.Mock; + let directLine: DirectLineStreaming; + + beforeEach(async () => { + jest.useFakeTimers({ now: 0 }); + + let token: string; + + [botProxy, { token }] = await Promise.all([ + setupBotProxy({ streamingBotURL: MOCKBOT3_URL }), + fetch(TOKEN_URL, { method: 'POST' }).then(res => res.json()) + ]); + + createAbortController = jest.fn(({ signal }) => { + const abortController = new AbortController(); + + signal.addEventListener('abort', () => abortController.abort(), { once: true }); + + return abortController; + }); + + activityObserver = mockObserver(); + connectionStatusObserver = mockObserver(); + directLine = new DirectLineStreaming({ + domain: botProxy.directLineStreamingURL, + token, + watchdog: ({ signal }) => createAbortController({ signal }).signal + }); + + directLine.connectionStatus$.subscribe(connectionStatusObserver); + }); + + afterEach(() => { + directLine.end(); + + jest.useRealTimers(); + }); + + describe('when connect', () => { + // WHEN: Connect. + beforeEach(() => directLine.activity$.subscribe(activityObserver)); + + // THEN: Should observe "Uninitialized" -> "Connecting" -> "Online". + test('should observe "Uninitialized" -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + + // WHEN: Connection status become "Online" and watchdog is created. + describe('after online and watchdog is connected', () => { + beforeEach(() => + waitFor( + () => { + expect(connectionStatusObserver.observe).toHaveBeenLastCalledWith([ + expect.any(Number), + 'next', + ConnectionStatus.Online + ]); + + expect(createAbortController).toBeCalledTimes(1); + }, + { timeout: 5_000 } + ) + ); + + // THEN: Should receive "Hello and welcome!" + test('should receive "Hello and welcome!"', () => + waitFor( + () => + expect(activityObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', expect.activityContaining('Hello and welcome!')] + ]), + { timeout: 5_000 } + )); + + // WHEN: Watchdog connection is closed. + describe('when watchdog detected a fault', () => { + beforeEach(() => { + const { + mock: { + results: [firstResult] + } + } = createAbortController; + + expect(firstResult).toHaveProperty('type', 'return'); + + firstResult.value.abort(); + }); + + // THEN: Should observe "Connecting" -> "Online" again. + test('should observe ... -> "Connecting" -> "Online"', () => + waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } + )); + + test('should recreate watchdog', () => + waitFor(() => expect(createAbortController).toBeCalledTimes(2), { timeout: 5_000 })); + }); + + describe('when connection is closed', () => { + beforeEach(() => directLine.end()); + + test('should abort the watchdog', () => { + expect(createAbortController).toHaveBeenCalledTimes(1); + expect(createAbortController.mock.calls[0][0]).toHaveProperty('signal.aborted', true); + }); + }); + }); + }); +}); From 1b373629f9d382f02f900ff8d6000a4240f27b51 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 21:48:19 -0700 Subject: [PATCH 09/33] Add abort watchdog test --- __tests__/directLineStreaming/watchdog.rest.story.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/__tests__/directLineStreaming/watchdog.rest.story.ts b/__tests__/directLineStreaming/watchdog.rest.story.ts index 94f3d4db..8ef7d454 100644 --- a/__tests__/directLineStreaming/watchdog.rest.story.ts +++ b/__tests__/directLineStreaming/watchdog.rest.story.ts @@ -115,6 +115,16 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { test('should reconnect watchdog', () => waitFor(() => expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(2), { timeout: 5_000 })); }); + + describe('when connection is closed', () => { + beforeEach(() => directLine.end()); + + test('should close the REST connection', () => + waitFor(() => { + expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(1); + expect(botProxy.numWatchdogConnection).toBe(0); + })); + }); }); }); }); From 99c460e4b0eafe3cdbee9c6320a11b4e322c8567 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 22:03:13 -0700 Subject: [PATCH 10/33] Add entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1845e77..257bbfeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Added + +- Direct Line Streaming: Added `watchdog` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) + ## [0.15.4] - 2023-06-05 ### Changed From a25f29297fdd3ff5867a4eb647d5b8a6c71ce718 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 22:14:31 -0700 Subject: [PATCH 11/33] Update comments --- src/directLineStreaming.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index b88f0ce4..b66c0356 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -30,26 +30,26 @@ interface DirectLineStreamingOptions { botAgent?: string; /** - * Sets the connection liveness probe. + * Sets the connection liveness probe for assisting detection of connection issues. * * When the probe detects any connection issues, the bot connection will be closed and treated as an error. * - * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` did not emit `error` event timely in case of connection issues. + * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` does not emit `error` event timely in case of connection issues. * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. * - * There are 3 ways to set the probe: `NetworkInformation` (via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API watchdog), or `function`. + * There are 3 ways to set the probe: `NetworkInformation` instance (using [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API watchdog), or `function`. * * When the probe is an instance of `NetworkInformation`: * * - When `change` event is received on the instance, the watchdog will treat it as a fault. * - * When the probe is an object, it will watch the liveness of a long-polling REST API: + * When the probe is an object, it will watch the liveness of a long-polling service via HTTP GET: * - * - `url` is the URL of a REST long-polling API. The REST API must keep the connection for a period of time and returns HTTP 2xx when it end. + * - `url` is the URL of a HTTP GET long-polling service. The service must keep the connection for a period of time and returns HTTP 2xx when the time has passed. * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. - * - If a non-2XX status code is received, it will be treated as a fault. - * - `minimumInterval` is the time to wait between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. - * It should be shorter than the time the API would keep the connection. + * - If a non-2xx status code is received, it will be treated as a fault. + * - `minimumInterval` is the time, in millisecond, to wait between pings. Minimum is 10 seconds, default to 25 seconds. + * The interval should be shorter than the time the long-polling API would keep the connection. * * When the probe is a function: * @@ -57,7 +57,7 @@ interface DirectLineStreamingOptions { * - The returned `AbortSignal` should be aborted as soon as the probe detects any connection issues. * - The function should create a new probe on every call and probe should not be reused. * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. - * - At any point of time, there should be no more than 1 probe active. The chat adapter is expected to signal the release of probe before requesting for a new one. + * - At any point of time, there should be no more than 1 probe active. The chat adapter will signal the release of probe before requesting for a new one. */ watchdog?: | NetworkInformation From 0d518d0ea311b8077915778884b8a2e454aa02f2 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 23:05:11 -0700 Subject: [PATCH 12/33] Add FAQ related to network change --- README.md | 80 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index bc24fb81..7a7fbdb6 100644 --- a/README.md +++ b/README.md @@ -6,23 +6,23 @@ [![Build Status](https://travis-ci.org/Microsoft/BotFramework-DirectLineJS.svg?branch=master)](https://travis-ci.org/Microsoft/BotFramework-DirectLineJS) -Client library for the [Microsoft Bot Framework](http://www.botframework.com) *[Direct Line](https://docs.botframework.com/en-us/restapi/directline3/)* protocol. +Client library for the [Microsoft Bot Framework](http://www.botframework.com) _[Direct Line](https://docs.botframework.com/en-us/restapi/directline3/)_ protocol. Used by [WebChat](https://github.com/Microsoft/BotFramework-WebChat) and thus (by extension) [Emulator](https://github.com/Microsoft/BotFramework-Emulator), WebChat channel, and [Azure Bot Service](https://azure.microsoft.com/en-us/services/bot-service/). ## FAQ -### *Who is this for?* +### _Who is this for?_ Anyone who is building a Bot Framework JavaScript client who does not want to use [WebChat](https://github.com/Microsoft/BotFramework-WebChat). If you're currently using WebChat, you don't need to make any changes as it includes this package. -### *What is that funny `subscribe()` method in the samples below?* +### _What is that funny `subscribe()` method in the samples below?_ Instead of callbacks or Promises, this library handles async operations using Observables. Try it, you'll like it! For more information, check out [RxJS](https://github.com/reactivex/rxjs/). -### *Can I use [TypeScript](http://www.typescriptlang.com)?* +### _Can I use [TypeScript](http://www.typescriptlang.com)?_ You bet. @@ -32,6 +32,18 @@ This is an official Microsoft-supported library, and is considered largely compl That said, the public API is still subject to change. +### Why the library did not detect Web Socket disconnections? + +On iOS/iPadOS, when network change from Wi-Fi to cellular (and vice versa), the `WebSocket` object will be stalled without any errors. This is not detectable nor workaroundable without any additional assistance. The issue is related to an experimental feature named "NSURLSession WebSocket". The feature is enabled by default on iOS/iPadOS 15 and up. + +Web developers can use an option named `watchdog` to assist the library to detect any connection issues. + +One effective method to detect connection issues is establishing a long-polling HTTP GET call to any service. The service would respond with HTTP 2xx and stream 1 byte of data using [chunked transfer encoding](https://en.wikipedia.org/wiki/Chunked_transfer_encoding). After the first chunk, the streaming connection should be kept on-hold for 30 seconds. Then, the service will send a final chunk and end the call gracefully. If network change did happen during the call, the streaming conection will be aborted prematurely and the watchdog will send a fault signal to the library. + +If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). Upon receiving the event, the watchdog will send a fault signal to the library. + +Lastly, web developers can also implement custom connection detection mechanism using [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal). When connection issues is detected, the `AbortSignal` will be aborted and the watchdog will signal a fault. + ## How to build from source 0. Clone this repo @@ -88,14 +100,16 @@ var directLine = new DirectLine({ ### Post activities to the bot: ```typescript -directLine.postActivity({ +directLine + .postActivity({ from: { id: 'myUserId', name: 'myUserName' }, // required (from.name is optional) type: 'message', text: 'a message for you, Rudy' -}).subscribe( - id => console.log("Posted activity, assigned ID ", id), - error => console.log("Error posting activity", error) -); + }) + .subscribe( + id => console.log('Posted activity, assigned ID ', id), + error => console.log('Error posting activity', error) + ); ``` You can also post messages with attachments, and non-message activities such as events, by supplying the appropriate fields in the activity. @@ -103,30 +117,23 @@ You can also post messages with attachments, and non-message activities such as ### Listen to activities sent from the bot: ```typescript -directLine.activity$ -.subscribe( - activity => console.log("received activity ", activity) -); +directLine.activity$.subscribe(activity => console.log('received activity ', activity)); ``` You can use RxJS operators on incoming activities. To see only message activities: ```typescript directLine.activity$ -.filter(activity => activity.type === 'message') -.subscribe( - message => console.log("received message ", message) -); + .filter(activity => activity.type === 'message') + .subscribe(message => console.log('received message ', message)); ``` Direct Line will helpfully send your client a copy of every sent activity, so a common pattern is to filter incoming messages on `from`: ```typescript directLine.activity$ -.filter(activity => activity.type === 'message' && activity.from.id === 'yourBotHandle') -.subscribe( - message => console.log("received message ", message) -); + .filter(activity => activity.type === 'message' && activity.from.id === 'yourBotHandle') + .subscribe(message => console.log('received message ', message)); ``` ### Monitor connection status @@ -134,19 +141,17 @@ directLine.activity$ Subscribing to either `postActivity` or `activity$` will start the process of connecting to the bot. Your app can listen to the connection status and react appropriately : ```typescript - import { ConnectionStatus } from 'botframework-directlinejs'; -directLine.connectionStatus$ -.subscribe(connectionStatus => { - switch(connectionStatus) { - case ConnectionStatus.Uninitialized: // the status when the DirectLine object is first created/constructed - case ConnectionStatus.Connecting: // currently trying to connect to the conversation - case ConnectionStatus.Online: // successfully connected to the converstaion. Connection is healthy so far as we know. - case ConnectionStatus.ExpiredToken: // last operation errored out with an expired token. Your app should supply a new one. - case ConnectionStatus.FailedToConnect: // the initial attempt to connect to the conversation failed. No recovery possible. - case ConnectionStatus.Ended: // the bot ended the conversation - } +directLine.connectionStatus$.subscribe(connectionStatus => { + switch (connectionStatus) { + case ConnectionStatus.Uninitialized: // the status when the DirectLine object is first created/constructed + case ConnectionStatus.Connecting: // currently trying to connect to the conversation + case ConnectionStatus.Online: // successfully connected to the converstaion. Connection is healthy so far as we know. + case ConnectionStatus.ExpiredToken: // last operation errored out with an expired token. Your app should supply a new one. + case ConnectionStatus.FailedToConnect: // the initial attempt to connect to the conversation failed. No recovery possible. + case ConnectionStatus.Ended: // the bot ended the conversation + } }); ``` @@ -168,7 +173,8 @@ directLine.reconnect(conversation); When using DirectLine with WebChat, closing the current tab or refreshing the page will create a new conversation in most cases. You can resume an existing conversation to keep the user in the same context. **When using a secret** you can resume a conversation by: -- Storing the conversationid (in a *permanent* place, like local storage) + +- Storing the conversationid (in a _permanent_ place, like local storage) - Giving this value back while creating the DirectLine object along with the secret ```typescript @@ -181,7 +187,8 @@ const dl = new DirectLine({ ``` **When using a token** you can resume a conversation by: -- Storing the conversationid and your token (in a *permanent* place, like local storage) + +- Storing the conversationid and your token (in a _permanent_ place, like local storage) - Calling the DirectLine reconnect API yourself to get a refreshed token and a streamurl - Creating the DirectLine object using the ConversationId, Token, and StreamUrl @@ -196,7 +203,7 @@ const dl = new DirectLine({ ``` **Getting any history that Direct Line has cached** : you can retrieve history using watermarks: -You can see the watermark as an *activity 'bookmark'*. The resuming scenario will replay all the conversation activities from the watermark you specify. +You can see the watermark as an _activity 'bookmark'_. The resuming scenario will replay all the conversation activities from the watermark you specify. ```typescript import { DirectLine } from 'botframework-directlinejs'; @@ -212,7 +219,7 @@ const dl = new DirectLine({ ## Contributing -This project welcomes contributions and suggestions. Most contributions require you to agree to a +This project welcomes contributions and suggestions. Most contributions require you to agree to a Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us the rights to use your contribution. For details, visit https://cla.microsoft.com. @@ -228,6 +235,7 @@ For more information see the [Code of Conduct FAQ](https://opensource.microsoft. contact [opencode@microsoft.com](mailto:opencode@microsoft.com) with any additional questions or comments. ## Reporting Security Issues + Security issues and bugs should be reported privately, via email, to the Microsoft Security Response Center (MSRC) at [secure@microsoft.com](mailto:secure@microsoft.com). You should receive a response within 24 hours. If for some reason you do not, please follow up via email to ensure we received your original message. Further information, including the [MSRC PGP](https://technet.microsoft.com/en-us/security/dn606155) key, can be found in the [Security TechCenter](https://technet.microsoft.com/en-us/security/default). Copyright (c) Microsoft Corporation. All rights reserved. From e1a5efc15d6ecc2845749c425971ebac38579653 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 5 Jul 2023 23:47:45 -0700 Subject: [PATCH 13/33] Rename watchdog to (network) probe --- CHANGELOG.md | 2 +- README.md | 8 +-- .../__setup__/createBotProxy.ts | 36 ++++++------ .../__setup__/setupBotProxy.ts | 12 ++-- ...tory.ts => networkProbe.function.story.ts} | 20 +++---- ... networkProbe.networkInformation.story.ts} | 8 +-- ...st.story.ts => networkProbe.rest.story.ts} | 34 +++++------ src/directLineStreaming.ts | 56 +++++++++---------- ...SocketClientWithProbe.integration.test.ts} | 24 ++++---- ...st.ts => WebSocketClientWithProbe.test.ts} | 28 +++++----- ...atchdog.ts => WebSocketClientWithProbe.ts} | 22 ++++---- src/streaming/probeNetworkInformation.test.ts | 25 +++++++++ ...ormation.ts => probeNetworkInformation.ts} | 10 ++-- .../{watchREST.test.ts => probeREST.test.ts} | 8 +-- src/streaming/{watchREST.ts => probeREST.ts} | 24 ++++---- src/streaming/watchNetworkInformation.test.ts | 25 --------- 16 files changed, 171 insertions(+), 171 deletions(-) rename __tests__/directLineStreaming/{watchdog.function.story.ts => networkProbe.function.story.ts} (88%) rename __tests__/directLineStreaming/{watchdog.networkInformation.story.ts => networkProbe.networkInformation.story.ts} (94%) rename __tests__/directLineStreaming/{watchdog.rest.story.ts => networkProbe.rest.story.ts} (75%) rename src/streaming/{WebSocketClientWithWatchdog.integration.test.ts => WebSocketClientWithProbe.integration.test.ts} (79%) rename src/streaming/{WebSocketClientWithWatchdog.test.ts => WebSocketClientWithProbe.test.ts} (85%) rename src/streaming/{WebSocketClientWithWatchdog.ts => WebSocketClientWithProbe.ts} (62%) create mode 100644 src/streaming/probeNetworkInformation.test.ts rename src/streaming/{watchNetworkInformation.ts => probeNetworkInformation.ts} (73%) rename src/streaming/{watchREST.test.ts => probeREST.test.ts} (97%) rename src/streaming/{watchREST.ts => probeREST.ts} (71%) delete mode 100644 src/streaming/watchNetworkInformation.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 257bbfeb..249fe895 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added -- Direct Line Streaming: Added `watchdog` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) +- Direct Line Streaming: Added `networkProbe` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) ## [0.15.4] - 2023-06-05 diff --git a/README.md b/README.md index 7a7fbdb6..a75370be 100644 --- a/README.md +++ b/README.md @@ -36,13 +36,13 @@ That said, the public API is still subject to change. On iOS/iPadOS, when network change from Wi-Fi to cellular (and vice versa), the `WebSocket` object will be stalled without any errors. This is not detectable nor workaroundable without any additional assistance. The issue is related to an experimental feature named "NSURLSession WebSocket". The feature is enabled by default on iOS/iPadOS 15 and up. -Web developers can use an option named `watchdog` to assist the library to detect any connection issues. +Web developers can use an option named `networkProbe` to assist the library to detect any connection issues. -One effective method to detect connection issues is establishing a long-polling HTTP GET call to any service. The service would respond with HTTP 2xx and stream 1 byte of data using [chunked transfer encoding](https://en.wikipedia.org/wiki/Chunked_transfer_encoding). After the first chunk, the streaming connection should be kept on-hold for 30 seconds. Then, the service will send a final chunk and end the call gracefully. If network change did happen during the call, the streaming conection will be aborted prematurely and the watchdog will send a fault signal to the library. +One effective method to detect connection issues is establishing a long-polling HTTP GET call to any service. The service would respond with HTTP 2xx and stream 1 byte of data using [chunked transfer encoding](https://en.wikipedia.org/wiki/Chunked_transfer_encoding). After the first chunk, the streaming connection should be kept on-hold for 30 seconds. Then, the service will send a final chunk and end the call gracefully. If network change did happen during the call, the streaming conection will be aborted prematurely and the probe will send a fault signal to the library. -If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). Upon receiving the event, the watchdog will send a fault signal to the library. +If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). Upon receiving the event, the probe will send a fault signal to the library. -Lastly, web developers can also implement custom connection detection mechanism using [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal). When connection issues is detected, the `AbortSignal` will be aborted and the watchdog will signal a fault. +Lastly, web developers can also implement custom connection detection mechanism using [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal). When connection issues is detected, the `AbortSignal` will be aborted and the probe will signal a fault. ## How to build from source diff --git a/__tests__/directLineStreaming/__setup__/createBotProxy.ts b/__tests__/directLineStreaming/__setup__/createBotProxy.ts index c7b9021f..beec3305 100644 --- a/__tests__/directLineStreaming/__setup__/createBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/createBotProxy.ts @@ -29,14 +29,14 @@ type CreateBotProxyInit = { type CreateBotProxyReturnValue = { cleanUp: () => void; - closeAllWatchdogConnections: () => void; + closeAllNetworkProbingConnections: () => void; closeAllWebSocketConnections: () => void; directLineStreamingURL: string; directLineURL: string; - watchdogURL: string; + networkProbeURL: string; - get numWatchdogConnection(): number; - get numOverTheLifetimeWatchdogConnection(): number; + get numNetworkProbingConnection(): number; + get numOverTheLifetimeNetworkProbingConnection(): number; }; const matchDirectLineStreamingProtocol = match('/.bot/', { decode: decodeURIComponent, end: false }); @@ -53,8 +53,8 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise void)[] = []; - let numOverTheLifetimeWatchdogConnection = 0; + const closeAllNetworkProbingConnections: (() => void)[] = []; + let numOverTheLifetimeNetworkProbingConnection = 0; streamingBotURL && app.use('/.bot/', createProxyMiddleware({ changeOrigin: true, logLevel: 'silent', target: streamingBotURL })); @@ -102,16 +102,16 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { + app.get('/test/network-probe', (req, res) => { const destroyResponse = () => { res.destroy(); }; - closeAllWatchdogConnections.push(destroyResponse); - numOverTheLifetimeWatchdogConnection++; + closeAllNetworkProbingConnections.push(destroyResponse); + numOverTheLifetimeNetworkProbingConnection++; req.once('error', destroyResponse); - res.once('close', () => removeInline(closeAllWatchdogConnections, destroyResponse)); + res.once('close', () => removeInline(closeAllNetworkProbingConnections, destroyResponse)); res.chunkedEncoding = true; res.statusCode = 200; @@ -206,21 +206,21 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { - closeAllWatchdogConnections.forEach(close => close()); - closeAllWatchdogConnections.splice(0); + closeAllNetworkProbingConnections: () => { + closeAllNetworkProbingConnections.forEach(close => close()); + closeAllNetworkProbingConnections.splice(0); }, closeAllWebSocketConnections, directLineStreamingURL: new URL('/.bot/v3/directline', url).href, directLineURL: new URL('/v3/directline', url).href, - watchdogURL: new URL('/test/watchdog', url).href, + networkProbeURL: new URL('/test/network-probe', url).href, - get numWatchdogConnection(): number { - return closeAllWatchdogConnections.length; + get numNetworkProbingConnection(): number { + return closeAllNetworkProbingConnections.length; }, - get numOverTheLifetimeWatchdogConnection(): number { - return numOverTheLifetimeWatchdogConnection; + get numOverTheLifetimeNetworkProbingConnection(): number { + return numOverTheLifetimeNetworkProbingConnection; } }); }); diff --git a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts index a4c066fa..54156ae1 100644 --- a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts @@ -19,18 +19,18 @@ export default async function setupBotProxy( botProxies.push(botProxy); return { - closeAllWatchdogConnections: botProxy.closeAllWatchdogConnections, + closeAllNetworkProbingConnections: botProxy.closeAllNetworkProbingConnections, closeAllWebSocketConnections: botProxy.closeAllWebSocketConnections, directLineURL: botProxy.directLineURL, directLineStreamingURL: botProxy.directLineStreamingURL, - watchdogURL: botProxy.watchdogURL, + networkProbeURL: botProxy.networkProbeURL, - get numWatchdogConnection() { - return botProxy.numWatchdogConnection; + get numNetworkProbingConnection() { + return botProxy.numNetworkProbingConnection; }, - get numOverTheLifetimeWatchdogConnection() { - return botProxy.numOverTheLifetimeWatchdogConnection; + get numOverTheLifetimeNetworkProbingConnection() { + return botProxy.numOverTheLifetimeNetworkProbingConnection; } }; } diff --git a/__tests__/directLineStreaming/watchdog.function.story.ts b/__tests__/directLineStreaming/networkProbe.function.story.ts similarity index 88% rename from __tests__/directLineStreaming/watchdog.function.story.ts rename to __tests__/directLineStreaming/networkProbe.function.story.ts index 0ac51280..41bdda9c 100644 --- a/__tests__/directLineStreaming/watchdog.function.story.ts +++ b/__tests__/directLineStreaming/networkProbe.function.story.ts @@ -16,8 +16,8 @@ const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directli jest.setTimeout(10_000); -// GIVEN: A Direct Line Streaming chat adapter with watchdog on REST API. -describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { +// GIVEN: A Direct Line Streaming chat adapter with network probe on REST API. +describe('Direct Line Streaming chat adapter with network probe on REST API', () => { let activityObserver: MockObserver; let botProxy: ResultOfPromise>; let connectionStatusObserver: MockObserver; @@ -46,8 +46,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { connectionStatusObserver = mockObserver(); directLine = new DirectLineStreaming({ domain: botProxy.directLineStreamingURL, - token, - watchdog: ({ signal }) => createAbortController({ signal }).signal + networkProbe: ({ signal }) => createAbortController({ signal }).signal, + token }); directLine.connectionStatus$.subscribe(connectionStatusObserver); @@ -75,8 +75,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - // WHEN: Connection status become "Online" and watchdog is created. - describe('after online and watchdog is connected', () => { + // WHEN: Connection status become "Online" and network probe is created. + describe('after online and network probe is connected', () => { beforeEach(() => waitFor( () => { @@ -102,8 +102,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - // WHEN: Watchdog connection is closed. - describe('when watchdog detected a fault', () => { + // WHEN: Network probing connection is closed. + describe('when the probing connection detected a fault', () => { beforeEach(() => { const { mock: { @@ -130,14 +130,14 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - test('should recreate watchdog', () => + test('should recreate network probe', () => waitFor(() => expect(createAbortController).toBeCalledTimes(2), { timeout: 5_000 })); }); describe('when connection is closed', () => { beforeEach(() => directLine.end()); - test('should abort the watchdog', () => { + test('should abort the network probe', () => { expect(createAbortController).toHaveBeenCalledTimes(1); expect(createAbortController.mock.calls[0][0]).toHaveProperty('signal.aborted', true); }); diff --git a/__tests__/directLineStreaming/watchdog.networkInformation.story.ts b/__tests__/directLineStreaming/networkProbe.networkInformation.story.ts similarity index 94% rename from __tests__/directLineStreaming/watchdog.networkInformation.story.ts rename to __tests__/directLineStreaming/networkProbe.networkInformation.story.ts index d7e59061..78bc874c 100644 --- a/__tests__/directLineStreaming/watchdog.networkInformation.story.ts +++ b/__tests__/directLineStreaming/networkProbe.networkInformation.story.ts @@ -16,8 +16,8 @@ const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directli jest.setTimeout(10_000); -// GIVEN: A Direct Line Streaming chat adapter with watchdog on Network Information API. -describe('Direct Line Streaming chat adapter with watchdog on Network Information API', () => { +// GIVEN: A Direct Line Streaming chat adapter with network probe on Network Information API. +describe('Direct Line Streaming chat adapter with network probe on Network Information API', () => { let activityObserver: MockObserver; let botProxy: ResultOfPromise>; let connectionStatusObserver: MockObserver; @@ -45,8 +45,8 @@ describe('Direct Line Streaming chat adapter with watchdog on Network Informatio connectionStatusObserver = mockObserver(); directLine = new DirectLineStreaming({ domain: botProxy.directLineStreamingURL, - token, - watchdog: navigator.connection + networkProbe: navigator.connection, + token }); directLine.connectionStatus$.subscribe(connectionStatusObserver); diff --git a/__tests__/directLineStreaming/watchdog.rest.story.ts b/__tests__/directLineStreaming/networkProbe.rest.story.ts similarity index 75% rename from __tests__/directLineStreaming/watchdog.rest.story.ts rename to __tests__/directLineStreaming/networkProbe.rest.story.ts index 8ef7d454..8edabae3 100644 --- a/__tests__/directLineStreaming/watchdog.rest.story.ts +++ b/__tests__/directLineStreaming/networkProbe.rest.story.ts @@ -16,8 +16,8 @@ const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directli jest.setTimeout(10_000); -// GIVEN: A Direct Line Streaming chat adapter with watchdog on REST API. -describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { +// GIVEN: A Direct Line Streaming chat adapter with network probe on REST API. +describe('Direct Line Streaming chat adapter with network probe on REST API', () => { let activityObserver: MockObserver; let botProxy: ResultOfPromise>; let connectionStatusObserver: MockObserver; @@ -37,8 +37,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { connectionStatusObserver = mockObserver(); directLine = new DirectLineStreaming({ domain: botProxy.directLineStreamingURL, - token, - watchdog: { url: botProxy.watchdogURL } + networkProbe: { url: botProxy.networkProbeURL }, + token }); directLine.connectionStatus$.subscribe(connectionStatusObserver); @@ -66,8 +66,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - // WHEN: Connection status become "Online" and watchdog is connected. - describe('after online and watchdog is connected', () => { + // WHEN: Connection status become "Online" and the network probe is connected. + describe('after online and the network probe is connected', () => { beforeEach(() => waitFor( () => { @@ -77,8 +77,8 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { ConnectionStatus.Online ]); - expect(botProxy.numWatchdogConnection).toBe(1); - expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(1); + expect(botProxy.numNetworkProbingConnection).toBe(1); + expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(1); }, { timeout: 5_000 } ) @@ -94,9 +94,9 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - // WHEN: Watchdog connection is closed. - describe('when watchdog failed', () => { - beforeEach(() => botProxy.closeAllWatchdogConnections()); + // WHEN: The network probing connection is forcibly closed. + describe('when the network probing connection is forcibly closed', () => { + beforeEach(() => botProxy.closeAllNetworkProbingConnections()); // THEN: Should observe "Connecting" -> "Online" again. test('should observe ... -> "Connecting" -> "Online"', () => @@ -112,17 +112,17 @@ describe('Direct Line Streaming chat adapter with watchdog on REST API', () => { { timeout: 5_000 } )); - test('should reconnect watchdog', () => - waitFor(() => expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(2), { timeout: 5_000 })); + test('should reconnect the network probe', () => + waitFor(() => expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(2), { timeout: 5_000 })); }); - describe('when connection is closed', () => { + describe('when the chat adapter is closed', () => { beforeEach(() => directLine.end()); - test('should close the REST connection', () => + test('should close the network probing connection', () => waitFor(() => { - expect(botProxy.numOverTheLifetimeWatchdogConnection).toBe(1); - expect(botProxy.numWatchdogConnection).toBe(0); + expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(1); + expect(botProxy.numNetworkProbingConnection).toBe(0); })); }); }); diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index b66c0356..d1e8bd58 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -8,9 +8,9 @@ import * as BFSE from 'botframework-streaming'; import createDeferred from './createDeferred'; import fetch from 'cross-fetch'; -import watchNetworkInformation from './streaming/watchNetworkInformation'; -import watchREST from './streaming/watchREST'; -import WebSocketClientWithWatchdog from './streaming/WebSocketClientWithWatchdog'; +import probeNetworkInformation from './streaming/probeNetworkInformation'; +import probeREST from './streaming/probeREST'; +import WebSocketClientWithProbe from './streaming/WebSocketClientWithProbe'; import type { Deferred } from './createDeferred'; import { Activity, ConnectionStatus, Conversation, IBotConnection, Media, Message } from './directLine'; @@ -30,20 +30,20 @@ interface DirectLineStreamingOptions { botAgent?: string; /** - * Sets the connection liveness probe for assisting detection of connection issues. + * Sets the network probe for assisting detection of connection issues. * * When the probe detects any connection issues, the bot connection will be closed and treated as an error. * * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` does not emit `error` event timely in case of connection issues. * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. * - * There are 3 ways to set the probe: `NetworkInformation` instance (using [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API watchdog), or `function`. + * There are 3 ways to set the probe: `NetworkInformation` instance (using [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API probe), or `function`. * * When the probe is an instance of `NetworkInformation`: * - * - When `change` event is received on the instance, the watchdog will treat it as a fault. + * - When `change` event is received on the instance, the probe will treat it as a fault. * - * When the probe is an object, it will watch the liveness of a long-polling service via HTTP GET: + * When the probe is an object, it will probe the liveness of a long-polling service via HTTP GET: * * - `url` is the URL of a HTTP GET long-polling service. The service must keep the connection for a period of time and returns HTTP 2xx when the time has passed. * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. @@ -53,13 +53,13 @@ interface DirectLineStreamingOptions { * * When the probe is a function: * - * - The function will be called when a connection is being established. Thus, a new liveness probe is needed. + * - The function will be called when a connection is being established. Thus, a new network probe is needed. * - The returned `AbortSignal` should be aborted as soon as the probe detects any connection issues. * - The function should create a new probe on every call and probe should not be reused. * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. * - At any point of time, there should be no more than 1 probe active. The chat adapter will signal the release of probe before requesting for a new one. */ - watchdog?: + networkProbe?: | NetworkInformation | { minimumInterval?: number; @@ -148,32 +148,32 @@ export class DirectLineStreaming implements IBotConnection { private _botAgent = ''; - #watchdog: ((init: { signal: AbortSignal }) => AbortSignal) | undefined; + #networkProbe: ((init: { signal: AbortSignal }) => AbortSignal) | undefined; constructor(options: DirectLineStreamingOptions) { - // Rectifies options.watchdog. - const watchdog = options?.watchdog; + // Rectifies options.probe. + const networkProbe = options?.networkProbe; - if (typeof watchdog === 'function') { - this.#watchdog = watchdog; - } else if (typeof watchdog === 'undefined') { + if (typeof networkProbe === 'function') { + this.#networkProbe = networkProbe; + } else if (typeof networkProbe === 'undefined') { // Intentionally left blank. } else if ( - watchdog instanceof EventTarget || + networkProbe instanceof EventTarget || // We also accept `EventTargetLike`. - (typeof watchdog['addEventListener'] === 'function' && typeof watchdog['removeEventListener'] === 'function') + (typeof networkProbe['addEventListener'] === 'function' && typeof networkProbe['removeEventListener'] === 'function') ) { - this.#watchdog = ({ signal }: { signal: AbortSignal }) => - watchNetworkInformation(watchdog as NetworkInformation, { signal }); + this.#networkProbe = ({ signal }: { signal: AbortSignal }) => + probeNetworkInformation(networkProbe as NetworkInformation, { signal }); } else if ( - (typeof watchdog.minimumInterval === 'number' || typeof watchdog.minimumInterval === 'undefined') && - (typeof watchdog.url === 'string' || watchdog.url instanceof URL) + (typeof networkProbe.minimumInterval === 'number' || typeof networkProbe.minimumInterval === 'undefined') && + (typeof networkProbe.url === 'string' || networkProbe.url instanceof URL) ) { - this.#watchdog = ({ signal }: { signal: AbortSignal }) => - watchREST(watchdog.url, { minimumInterval: watchdog.minimumInterval, signal }); + this.#networkProbe = ({ signal }: { signal: AbortSignal }) => + probeREST(networkProbe.url, { minimumInterval: networkProbe.minimumInterval, signal }); } else { throw new Error( - 'botframework-directlinejs: "watchdog" option must be either a function returning an AbortSignal, an object, or undefined.' + 'botframework-directlinejs: "networkProbe" option must be either a function returning an AbortSignal, an object, or undefined.' ); } @@ -399,11 +399,12 @@ export class DirectLineStreaming implements IBotConnection { // This promise will resolve when it is disconnected. return new Promise(async (resolve, reject) => { try { - const watchdog: AbortSignal | undefined = - typeof this.#watchdog === 'function' ? this.#watchdog({ signal: abortController.signal }) : undefined; + const probe: AbortSignal | undefined = + typeof this.#networkProbe === 'function' ? this.#networkProbe({ signal: abortController.signal }) : undefined; - this.streamConnection = new WebSocketClientWithWatchdog({ + this.streamConnection = new WebSocketClientWithProbe({ disconnectionHandler: resolve, + probe, requestHandler: { processRequest: streamingRequest => { // If `streamConnection` is still current, allow call to `processRequest()`, otherwise, ignore calls to `processRequest()`. @@ -416,7 +417,6 @@ export class DirectLineStreaming implements IBotConnection { } }, url: wsUrl, - watchdog }); this.queueActivities = true; diff --git a/src/streaming/WebSocketClientWithWatchdog.integration.test.ts b/src/streaming/WebSocketClientWithProbe.integration.test.ts similarity index 79% rename from src/streaming/WebSocketClientWithWatchdog.integration.test.ts rename to src/streaming/WebSocketClientWithProbe.integration.test.ts index 4d146124..9cf892cd 100644 --- a/src/streaming/WebSocketClientWithWatchdog.integration.test.ts +++ b/src/streaming/WebSocketClientWithProbe.integration.test.ts @@ -1,5 +1,5 @@ import type { WebSocketClient } from 'botframework-streaming'; -import type OriginalWebSocketClientWithWatchdog from './WebSocketClientWithWatchdog'; +import type OriginalWebSocketClientWithProbe from './WebSocketClientWithProbe'; // Mocked modules are available across the test file. They cannot be unmocked. // Thus, they are more-or-less similar to import/require. @@ -20,20 +20,20 @@ const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: const url: string = 'wss://dummy/'; let client: WebSocketClient; -let watchdog: AbortController; +let probe: AbortController; beforeEach(() => { - watchdog = new AbortController(); + probe = new AbortController(); - let WebSocketClientWithWatchdog: typeof OriginalWebSocketClientWithWatchdog; + let WebSocketClientWithProbe: typeof OriginalWebSocketClientWithProbe; - WebSocketClientWithWatchdog = require('./WebSocketClientWithWatchdog').default; + WebSocketClientWithProbe = require('./WebSocketClientWithProbe').default; - client = new WebSocketClientWithWatchdog({ + client = new WebSocketClientWithProbe({ disconnectionHandler, + probe: probe.signal, requestHandler, - url, - watchdog: watchdog.signal + url }); // Spy on all `console.warn()`. @@ -53,16 +53,16 @@ describe('call connect()', () => { // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); - describe('followed by aborting watchdog', () => { - beforeEach(() => watchdog.abort()); + describe('followed by aborting the probe', () => { + beforeEach(() => probe.abort()); // After disconnected() is called, there should be no extra calls for aborting the signal. test('should have no extra calls to disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(2)); }); }); - describe('abort watchdog', () => { - beforeEach(() => watchdog.abort()); + describe('abort the probe', () => { + beforeEach(() => probe.abort()); // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); diff --git a/src/streaming/WebSocketClientWithWatchdog.test.ts b/src/streaming/WebSocketClientWithProbe.test.ts similarity index 85% rename from src/streaming/WebSocketClientWithWatchdog.test.ts rename to src/streaming/WebSocketClientWithProbe.test.ts index b6eeff72..0e6dd1ee 100644 --- a/src/streaming/WebSocketClientWithWatchdog.test.ts +++ b/src/streaming/WebSocketClientWithProbe.test.ts @@ -1,5 +1,5 @@ import type { WebSocketClient } from 'botframework-streaming'; -import type ActualWebSocketClientWithWatchdog from './WebSocketClientWithWatchdog'; +import type ActualWebSocketClientWithProbe from './WebSocketClientWithProbe'; // Mocked modules are available across the test file. They cannot be unmocked. // Thus, they are more-or-less similar to import/require. @@ -59,20 +59,20 @@ const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: const url: string = 'wss://dummy/'; let client: WebSocketClient; -let watchdog: AbortController; +let probe: AbortController; beforeEach(() => { - watchdog = new AbortController(); + probe = new AbortController(); - let WebSocketClientWithWatchdog: typeof ActualWebSocketClientWithWatchdog; + let WebSocketClientWithProbe: typeof ActualWebSocketClientWithProbe; - WebSocketClientWithWatchdog = require('./WebSocketClientWithWatchdog').default; + WebSocketClientWithProbe = require('./WebSocketClientWithProbe').default; - client = new WebSocketClientWithWatchdog({ + client = new WebSocketClientWithProbe({ disconnectionHandler, + probe: probe.signal, requestHandler, - url, - watchdog: watchdog.signal + url }); // Spy on all `console.warn()`. @@ -105,8 +105,8 @@ describe('connect()', () => { test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); }); - describe('then abort watchdog', () => { - beforeEach(() => watchdog.abort()); + describe('then abort the probe', () => { + beforeEach(() => probe.abort()); test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); @@ -125,14 +125,14 @@ describe('connect()', () => { }); }); -describe('watchdog is aborted then call connect()', () => { +describe('probe is aborted then call connect()', () => { beforeEach(() => { - watchdog.abort(); + probe.abort(); client.connect(); }); test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); - test('should warn "watchdog is aborted before connect()"', () => - expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('watchdog is aborted before connect()'))); + test('should warn "probe is aborted before connect()"', () => + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); }); diff --git a/src/streaming/WebSocketClientWithWatchdog.ts b/src/streaming/WebSocketClientWithProbe.ts similarity index 62% rename from src/streaming/WebSocketClientWithWatchdog.ts rename to src/streaming/WebSocketClientWithProbe.ts index a601deb8..f71ff6a9 100644 --- a/src/streaming/WebSocketClientWithWatchdog.ts +++ b/src/streaming/WebSocketClientWithProbe.ts @@ -2,43 +2,43 @@ import { WebSocketClient } from 'botframework-streaming'; import type { RequestHandler } from 'botframework-streaming'; -type WebSocketClientWithWatchdogInit = { +type WebSocketClientWithProbeInit = { /** * Gets or sets the observer function for disconnection or error sending/receiving through WebSocket. * * Note: This function could be called multiple times, the callee is expected to ignore subsequent calls. */ disconnectionHandler?: (message: string) => void; + probe?: AbortSignal; requestHandler: RequestHandler; url: string; - watchdog?: AbortSignal; }; -export default class WebSocketClientWithWatchdog extends WebSocketClient { - constructor({ disconnectionHandler, requestHandler, url, watchdog }: WebSocketClientWithWatchdogInit) { +export default class WebSocketClientWithProbe extends WebSocketClient { + constructor({ disconnectionHandler, probe, requestHandler, url }: WebSocketClientWithProbeInit) { super({ disconnectionHandler, requestHandler, url }); - this.#watchdog = watchdog; + this.#probe = probe; } #connectCalled: boolean = false; - #watchdog: WebSocketClientWithWatchdogInit['watchdog']; + #probe: WebSocketClientWithProbeInit['probe']; - // TODO: Better, the `watchdog` should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. + // TODO: Better, the `probe` should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. // `BrowserWebSocket` is where it creates `WebSocket` object. - // The `watchdog` object should accompany `WebSocket` and forcibly close it on abort. + // The `probe` object should accompany `WebSocket` and forcibly close it on abort. // Maybe `botframework-streaming` should accept ponyfills. connect(): Promise { if (this.#connectCalled) { console.warn('botframework-directlinejs: connect() can only be called once.'); return; - } else if (this.#watchdog?.aborted) { - console.warn('botframework-directlinejs: watchdog is aborted before connect().'); + } else if (this.#probe?.aborted) { + console.warn('botframework-directlinejs: probe is aborted before connect().'); return; } @@ -48,7 +48,7 @@ export default class WebSocketClientWithWatchdog extends WebSocketClient { // Note: It is expected disconnectionHandler() will be called multiple times. // If we call disconnect(), both sender/receiver will call super.onConnectionDisconnected(), // which in turn, call disconnectionHandler() twice, all from a single disconnect() call. - this.#watchdog?.addEventListener('abort', () => this.disconnect(), { once: true }); + this.#probe?.addEventListener('abort', () => this.disconnect(), { once: true }); return super.connect(); } diff --git a/src/streaming/probeNetworkInformation.test.ts b/src/streaming/probeNetworkInformation.test.ts new file mode 100644 index 00000000..cd44a9e3 --- /dev/null +++ b/src/streaming/probeNetworkInformation.test.ts @@ -0,0 +1,25 @@ +import probeNetworkInformation from './probeNetworkInformation'; + +let abortController: AbortController; +let networkInformation: EventTarget; + +beforeEach(() => { + abortController = new AbortController(); + networkInformation = new EventTarget(); +}); + +describe('after constructed', () => { + let probe: AbortSignal; + + beforeEach(() => { + probe = probeNetworkInformation(networkInformation, { signal: abortController.signal }); + }); + + test('should not be aborted', () => expect(probe.aborted).toBe(false)); + + describe('when "change" event is received', () => { + beforeEach(() => networkInformation.dispatchEvent(new Event('change'))); + + test('should be aborted', () => expect(probe.aborted).toBe(true)); + }); +}); diff --git a/src/streaming/watchNetworkInformation.ts b/src/streaming/probeNetworkInformation.ts similarity index 73% rename from src/streaming/watchNetworkInformation.ts rename to src/streaming/probeNetworkInformation.ts index 63021a56..e7f2779b 100644 --- a/src/streaming/watchNetworkInformation.ts +++ b/src/streaming/probeNetworkInformation.ts @@ -19,18 +19,18 @@ declare global { } } -type WatchNetworkInformationInit = { +type ProbeNetworkInformationInit = { signal: AbortSignal; }; /** - * Watches the connection a device is using to communicate with the network via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). + * Probes the connection a device is using to communicate with the network via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). * - * When the connection change, watchdog will treat it as a fault. + * When the connection change, the probe will treat it as a fault. */ -export default function watchNetworkInformation( +export default function probeNetworkInformation( connection: NetworkInformation, - { signal }: WatchNetworkInformationInit + { signal }: ProbeNetworkInformationInit ): AbortSignal { const abortController = new AbortController(); const handleNetworkInformationChange = () => abortController.abort(); diff --git a/src/streaming/watchREST.test.ts b/src/streaming/probeREST.test.ts similarity index 97% rename from src/streaming/watchREST.test.ts rename to src/streaming/probeREST.test.ts index 55db6970..95b7a57a 100644 --- a/src/streaming/watchREST.test.ts +++ b/src/streaming/probeREST.test.ts @@ -3,8 +3,8 @@ import express from 'express'; import hasResolved from 'has-resolved'; import createDeferred from '../createDeferred'; +import probeREST from './probeREST'; import waitFor from './__setup__/waitFor'; -import watchREST from './watchREST'; import type { Deferred } from '../createDeferred'; import type { IncomingMessage, Server, ServerResponse } from 'http'; @@ -101,7 +101,7 @@ afterEach(() => { describe('with default options', () => { beforeEach(() => { - signal = watchREST(serverURL, { signal: abortController.signal }); + signal = probeREST(serverURL, { signal: abortController.signal }); }); describe('after first poll received', () => { @@ -155,7 +155,7 @@ describe('with default options', () => { describe('with minimumInterval=45_000', () => { beforeEach(() => { - signal = watchREST(serverURL, { + signal = probeREST(serverURL, { minimumInterval: 45_000, signal: abortController.signal }); @@ -195,7 +195,7 @@ describe('with minimumInterval=45_000', () => { describe('with minimumInterval=15_000', () => { beforeEach(() => { - signal = watchREST(serverURL, { + signal = probeREST(serverURL, { minimumInterval: 15_000, signal: abortController.signal }); diff --git a/src/streaming/watchREST.ts b/src/streaming/probeREST.ts similarity index 71% rename from src/streaming/watchREST.ts rename to src/streaming/probeREST.ts index ee6905dc..4fdef6e1 100644 --- a/src/streaming/watchREST.ts +++ b/src/streaming/probeREST.ts @@ -1,49 +1,49 @@ import sleep from './sleep'; -// The watchdog service should respond HTTP 2xx not sooner than 30 seconds. -// To prevent DoS the watchdog service, we should not send ping the watchdog service sooner than 25 seconds. +// The liveness service should respond HTTP 2xx not sooner than 30 seconds. +// To prevent DoS the liveness service, we should not send probe to the liveness service sooner than 25 seconds. let DEFAULT_MINIMUM_INTERVAL = 25_000; let MINIMUM_MINIMUM_INTERVAL = 10_000; -type WatchRESTInit = { +type ProbeRESTInit = { /** * Minimum time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. * - * The watching REST API endpoint should keep the polling call open for at least 30 seconds, then respond with HTTP 2xx. + * The probing REST API endpoint should keep the polling call open for at least 30 seconds, then respond with HTTP 2xx. * * If the service respond with HTTP 2xx sooner than this interval, the next call will be delayed until the interval has passed. - * In the meantime, the watchdog will not be able to detect any connection faults. + * In the meantime, the probe will not be able to detect any connection faults. * * If the service respond with HTTP 2xx on or later than this interval, the next call will be made immediately. * * For example, assumes the interval is set to 25 seconds. If the service responded after 10 seconds the call is being made, - * the watchdog will wait for 15 seconds before making another call. + * the probe will wait for 15 seconds before making another call. */ minimumInterval?: number; /** - * Signal to abort the watchdog. + * Signal to abort the probe. * - * When this signal is being aborted, the watchdog will treat it as a fault. + * When this signal is being aborted, the probe will treat it as a fault. */ signal?: AbortSignal; }; /** - * Watches connectivity issues by continuously calling a long-polling REST API endpoint and returns an `AbortSignal` object. + * Probes connectivity issues by continuously calling a long-polling REST API endpoint and returns an `AbortSignal` object. * * The returned `AbortSignal` object will be aborted when: * * 1. the REST API endpoint did not return a HTTP 2xx status, or; - * 2. the `WatchRESTInit.signal` is aborted. + * 2. the `ProbeRESTInit.signal` is aborted. * * The REST API endpoint should keep the polling call open for 30 seconds, then respond with HTTP 2xx. * - * Upon receiving HTTP 2xx, the watchdog will issue another long polling call immediately and not sooner than `minimumInterval`. + * Upon receiving HTTP 2xx, the probe will issue another long polling call immediately and not sooner than `minimumInterval`. * * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends using a timeout value of 30 seconds. */ -export default function watchREST(url: string | URL, { minimumInterval, signal }: WatchRESTInit = {}): AbortSignal { +export default function probeREST(url: string | URL, { minimumInterval, signal }: ProbeRESTInit = {}): AbortSignal { const abortController = new AbortController(); (async () => { diff --git a/src/streaming/watchNetworkInformation.test.ts b/src/streaming/watchNetworkInformation.test.ts deleted file mode 100644 index b95cd3ff..00000000 --- a/src/streaming/watchNetworkInformation.test.ts +++ /dev/null @@ -1,25 +0,0 @@ -import watchNetworkInformation from './watchNetworkInformation'; - -let abortController: AbortController; -let networkInformation: EventTarget; - -beforeEach(() => { - abortController = new AbortController(); - networkInformation = new EventTarget(); -}); - -describe('after constructed', () => { - let watchdog: AbortSignal; - - beforeEach(() => { - watchdog = watchNetworkInformation(networkInformation, { signal: abortController.signal }); - }); - - test('should not be aborted', () => expect(watchdog.aborted).toBe(false)); - - describe('when "change" event is received', () => { - beforeEach(() => navigator.connection.dispatchEvent(new Event('change'))); - - test('should be aborted', () => expect(watchdog.aborted).toBe(true)); - }); -}); From d6d78e1d7ac1f6005ad2342b9dc7bd96cb1f4359 Mon Sep 17 00:00:00 2001 From: William Wong Date: Sat, 8 Jul 2023 17:00:28 -0700 Subject: [PATCH 14/33] Add NetworkInformationObserver --- src/streaming/NetworkInformation.d.ts | 24 ++ .../NetworkInformationObserver.test.ts | 367 ++++++++++++++++++ src/streaming/NetworkInformationObserver.ts | 154 ++++++++ src/streaming/mergeAbortSignal.ts | 19 + src/streaming/probeNetworkInformation.ts | 21 +- 5 files changed, 565 insertions(+), 20 deletions(-) create mode 100644 src/streaming/NetworkInformation.d.ts create mode 100644 src/streaming/NetworkInformationObserver.test.ts create mode 100644 src/streaming/NetworkInformationObserver.ts create mode 100644 src/streaming/mergeAbortSignal.ts diff --git a/src/streaming/NetworkInformation.d.ts b/src/streaming/NetworkInformation.d.ts new file mode 100644 index 00000000..12c6e96f --- /dev/null +++ b/src/streaming/NetworkInformation.d.ts @@ -0,0 +1,24 @@ +declare global { + // This is subset of https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation. + interface NetworkInformation extends EventTarget { + addEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + + removeEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + + get type(): 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; + } + + interface Navigator { + get connection(): NetworkInformation; + } +} + +export {} diff --git a/src/streaming/NetworkInformationObserver.test.ts b/src/streaming/NetworkInformationObserver.test.ts new file mode 100644 index 00000000..c71ff6a4 --- /dev/null +++ b/src/streaming/NetworkInformationObserver.test.ts @@ -0,0 +1,367 @@ +/// + +import { createServer, IncomingMessage } from 'http'; +import express from 'express'; + +import NetworkInformationObserver from './NetworkInformationObserver'; +import waitFor from './__setup__/waitFor'; + +import type { Response } from 'express'; +import type { Socket } from 'net'; + +beforeEach(() => jest.useFakeTimers({ advanceTimers: true, now: 0 })); +afterEach(() => jest.useRealTimers()); + +let baseURL: URL; +let firstChunkInterval: number; +let processRequest: jest.Mock; +let server: ReturnType; +let sockets: Set; + +beforeEach(async () => { + const app = express(); + + firstChunkInterval = 0; + sockets = new Set(); + processRequest = jest.fn(); + + app.get('/api/poll', (req, res) => { + processRequest(req, res); + sockets.add(req.socket); + + req.once('close', () => { + res.destroy(); + sockets.delete(req.socket); + }); + + const timeout = +(new URL(req.url, 'http://localhost/').searchParams.get('timeout') || 0); + + res.statusCode = 200; + res.setHeader('cache-control', 'no-store'); + res.setHeader('transfer-encoding', 'chunked'); + + if (timeout) { + setTimeout(() => { + res.write(' '); + + setTimeout(() => res.end(), timeout * 1_000 - firstChunkInterval); + }, firstChunkInterval); + } else { + res.end(); + } + }); + + app.get('/api/shortpoll', (req, res) => { + processRequest(req, res); + + res.statusCode = 204; + res.setHeader('cache-control', 'no-store'); + res.end(); + }); + + app.get('/api/longpoll', (req, res) => { + processRequest(req, res); + sockets.add(req.socket); + + res.statusCode = 200; + res.setHeader('cache-control', 'no-store'); + res.setHeader('transfer-encoding', 'chunked'); + res.write(' '); + + req.once('close', () => { + res.destroy(); + sockets.delete(req.socket); + }); + + setTimeout(() => res.end(), 30_000); + }); + + server = createServer(app); + + await new Promise(resolve => server.listen(0, 'localhost', resolve)); + + const address = server.address(); + + if (!address) { + throw new Error('Cannot listen.'); + } + + baseURL = new URL(`http://${typeof address === 'string' ? address : `${address.address}:${address.port}`}`); +}); + +afterEach(() => { + server.closeAllConnections(); + server.close(); + + jest.resetAllMocks(); +}); + +describe('A NetworkInformationObserver', () => { + let callback: jest.Mock; + let observer: NetworkInformationObserver; + + beforeEach(() => { + observer = new NetworkInformationObserver((callback = jest.fn())); + }); + + describe.each<[string, string, string?]>([ + ['default options', '/api/poll'], + ['shortPollURL', '/api/longpoll', '/api/shortpoll'] + ])('observe with %s', (_, url, shortPollURL) => { + let expectedLongPollURL: URL; + let expectedShortPollURL: URL; + + beforeEach(() => { + if (shortPollURL) { + observer.observe(new URL(url, baseURL).href, { shortPollURL: new URL(shortPollURL, baseURL) }); + + expectedLongPollURL = new URL(url, baseURL); + expectedShortPollURL = new URL(shortPollURL, baseURL); + } else { + observer.observe(new URL(url, baseURL).href); + + expectedLongPollURL = new URL(url, baseURL); + expectedLongPollURL.searchParams.set('timeout', '30'); + expectedShortPollURL = new URL(url, baseURL); + expectedShortPollURL.searchParams.set('timeout', '0'); + } + }); + + afterEach(() => observer.disconnect()); + + describe('when callback is received', () => { + let connection: NetworkInformation; + let handleChange: jest.Mock; + + beforeEach(() => { + expect(callback).toBeCalledTimes(1); + connection = callback.mock.calls[0][0]; + connection.addEventListener('change', (handleChange = jest.fn())); + }); + + test('should receive an instance of NetworkInformation', () => { + expect(callback).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + type: expect.stringMatching(/^(bluetooth|cellular|ethernet|none|other|unknown|wifi|wimax)$/) + }) + ); + expect(typeof callback.mock.calls[0][0].addEventListener).toBe('function'); + expect(typeof callback.mock.calls[0][0].removeEventListener).toBe('function'); + }); + + describe('when connection is detected', () => { + beforeEach(() => + waitFor(() => { + expect(connection).toHaveProperty('type', 'unknown'); + expect(processRequest).toHaveBeenCalledTimes(2); + }) + ); + + test('should have type of "unknown"', () => + waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); + test('should dispatch "change" event', () => waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); + test('should connect with short poll followed by long poll', () => + waitFor(() => { + expect(processRequest).toHaveBeenCalledTimes(2); + expect(processRequest).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ url: expectedShortPollURL.pathname + expectedShortPollURL.search }), + expect.anything() + ); + expect(processRequest).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), + expect.anything() + ); + expect(sockets).toHaveProperty('size', 1); + })); + + describe('when connection is broken', () => { + beforeEach(() => { + for (const socket of sockets.values()) { + socket.destroy(); + } + }); + + test('should have type of "none"', () => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); + test('should dispatch "change" event', () => waitFor(() => expect(handleChange).toHaveBeenCalledTimes(2))); + + describe('after disconnection is detected', () => { + beforeEach(() => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); + + describe('after 1 second', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(1_000)); + + test('should not reconnect', () => expect(sockets).toHaveProperty('size', 0)); + }); + + describe('after 2 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(2_000)); + + test('should have type of "unknown"', () => + waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); + test('should dispatch "change" event', () => + waitFor(() => expect(handleChange).toHaveBeenCalledTimes(3))); + test('should call short-poll followed by long-poll', () => + waitFor(() => { + expect(processRequest).toHaveBeenCalledTimes(4); + expect(processRequest).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ url: expectedShortPollURL.pathname + expectedShortPollURL.search }), + expect.anything() + ); + expect(processRequest).toHaveBeenNthCalledWith( + 4, + expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), + expect.anything() + ); + expect(sockets).toHaveProperty('size', 1); + })); + }); + }); + }); + + describe('after 30 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(30_000)); + + test('should not dispatch additional "change" event', () => + waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); + test('should connect with long poll', () => + waitFor(() => { + expect(processRequest).toHaveBeenCalledTimes(3); + expect(processRequest).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), + expect.anything() + ); + expect(sockets).toHaveProperty('size', 1); + })); + + describe('after another 30 seconds', () => { + beforeEach(async () => { + await waitFor(() => expect(processRequest).toHaveBeenCalledTimes(3)); + await jest.advanceTimersByTimeAsync(30_000); + }); + + test('should not dispatch additional "change" event', () => + waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); + test('should connect with long poll', () => + waitFor(() => { + expect(processRequest).toHaveBeenCalledTimes(4); + expect(processRequest).toHaveBeenNthCalledWith( + 4, + expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), + expect.anything() + ); + expect(sockets).toHaveProperty('size', 1); + })); + }); + }); + }); + }); + }); + + describe('observe with first chunk delayed for 10 seconds', () => { + let connection: NetworkInformation; + + beforeEach(async () => { + firstChunkInterval = 10_000; + observer.observe(new URL('/api/poll', baseURL).href); + + await expect(callback).toBeCalledTimes(1); + connection = callback.mock.calls[0][0]; + }); + + afterEach(() => observer.disconnect()); + + test('should connect long poll', () => waitFor(() => expect(processRequest).toBeCalledTimes(2))); + + describe('when online', () => { + beforeEach(() => waitFor(() => expect(processRequest).toBeCalledTimes(2))); + + test('type should become "unknown"', () => waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); + + describe('after 5 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(5_000)); + + test('type should become "none"', () => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); + test('should have closed the connection', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); + }); + }); + }); + + describe('observe and online', () => { + let connection: NetworkInformation; + + beforeEach(async () => { + observer.observe(new URL('/api/poll', baseURL).href); + + await expect(callback).toBeCalledTimes(1); + connection = callback.mock.calls[0][0]; + + await waitFor(() => expect(processRequest).toBeCalledTimes(2)); + waitFor(() => expect(connection).toHaveProperty('type', 'unknown')); + waitFor(() => expect(sockets).toHaveProperty('size', 1)); + }); + + afterEach(() => observer.disconnect()); + + describe('when unobserve() is called', () => { + beforeEach(() => observer.unobserve(new URL('/api/poll', baseURL).href)); + + test('should disconnect the call', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); + }); + + describe('when disconnect() is called', () => { + beforeEach(() => observer.disconnect()); + + test('should disconnect the call', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); + }); + }); + + describe('observe with short polling API', () => { + let connection: NetworkInformation; + + beforeEach(async () => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + observer.observe(new URL('/api/shortpoll', baseURL).href); + + await expect(callback).toBeCalledTimes(1); + connection = callback.mock.calls[0][0]; + + await waitFor(() => expect(processRequest).toBeCalledTimes(2)); + waitFor(() => expect(connection).toHaveProperty('type', 'unknown')); + waitFor(() => expect(sockets).toHaveProperty('size', 1)); + }); + + afterEach(() => console.warn['mockRestore']?.()); + + test('should warn', () => { + expect(console.warn).toBeCalledTimes(1); + expect(console.warn).toHaveBeenNthCalledWith( + 1, + expect.stringContaining('should not return sooner than 10 seconds') + ); + }); + + describe('after 5 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(5_000)); + + test('should not reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(2))); + }); + + describe.only('after 10 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(10_000)); + + test('should reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(3))); + }); + + describe.only('after 20 seconds', () => { + beforeEach(() => jest.advanceTimersByTimeAsync(20_000)); + + test('should reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(4))); + }); + }); +}); diff --git a/src/streaming/NetworkInformationObserver.ts b/src/streaming/NetworkInformationObserver.ts new file mode 100644 index 00000000..2848ce76 --- /dev/null +++ b/src/streaming/NetworkInformationObserver.ts @@ -0,0 +1,154 @@ +import mergeAbortSignal from './mergeAbortSignal'; +import sleep from './sleep'; + +type NetworkInformationObserverCallback = (networkInformation: NetworkInformation) => void; +// type NetworkInformationType = 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; +type ObserveInit = { firstChunkWithin?: number; shortPollURL?: undefined | URL }; + +const DEFAULT_FIRST_CHUNK_WITHIN = 5_000; +const MINIMUM_PING_INTERVAL = 10_000; +const SLEEP_INTERVAL_AFTER_ERROR = 2_000; +const TIMEOUT_PARAM_NAME = 'timeout'; + +class NetworkInformationPolyfill extends EventTarget implements NetworkInformation { + constructor(url: URL, init: Required & { signal: AbortSignal }) { + super(); + + this.#firstChunkWithin = init.firstChunkWithin; + this.#shortPollURL = init.shortPollURL; + this.#signal = init.signal; + this.#type = 'none'; + this.#url = url; + + this.#start().catch(() => {}); + } + + #firstChunkWithin: number; + #shortPollURL: URL; + #signal: AbortSignal; + #type: 'none' | 'unknown'; + #url: URL; + + get type() { + return this.#type; + } + + #setOffline() { + this.#setType('none'); + } + + #setOnline() { + this.#setType('unknown'); + } + + #setType(type: 'none' | 'unknown') { + if (this.#type !== type) { + this.#type = type; + this.dispatchEvent(new Event('change')); + } + } + + async #start() { + let shortPing = true; + + while (!this.#signal.aborted) { + const currentAbortController = new AbortController(); + + try { + const signal = mergeAbortSignal(currentAbortController.signal, this.#signal); + const startTime = Date.now(); + + const res = await Promise.race([ + fetch(shortPing ? this.#shortPollURL : this.#url, { + headers: { 'cache-control': 'no-store' }, + signal + }), + sleep(this.#firstChunkWithin, { signal }).then(() => + Promise.reject(new Error('Timed out while waiting for first chunk.')) + ) + ]); + + // Received first chunk. + this.#setOnline(); + await res.arrayBuffer(); + + const timeToSleep = Math.max(0, MINIMUM_PING_INTERVAL + startTime - Date.now()); + + // Do not ping long-polling sooner than 10 seconds. + if (!shortPing) { + if (timeToSleep > 0) { + console.warn( + `botframework-directlinejs: network connectivity probe should not return sooner than 10 seconds.` + ); + + await sleep(timeToSleep, { signal }); + } + } + + shortPing = false; + } catch (error) { + currentAbortController.abort(); + + this.#setOffline(); + shortPing = true; + + await sleep(SLEEP_INTERVAL_AFTER_ERROR, { signal: this.#signal }); + } + } + } +} + +export default class NetworkInformationObserver { + constructor(callback: NetworkInformationObserverCallback) { + this.#callback = callback; + } + + #callback: NetworkInformationObserverCallback; + #connections: Map = new Map(); + + disconnect() { + for (const [_, abortController] of this.#connections.values()) { + abortController.abort(); + } + + this.#connections.clear(); + } + + observe(url: string, init: ObserveInit = {}) { + let entry = this.#connections.get(url); + + if (!entry) { + const rectifiedFirstChunkWithin = init.firstChunkWithin || DEFAULT_FIRST_CHUNK_WITHIN; + let rectifiedShortPollURL: URL; + let rectifiedURL: URL; + + if (init.shortPollURL) { + rectifiedShortPollURL = new URL(init.shortPollURL); + rectifiedURL = new URL(url); + } else { + rectifiedShortPollURL = new URL(url); + rectifiedShortPollURL.searchParams.set(TIMEOUT_PARAM_NAME, '0'); + rectifiedURL = new URL(url); + rectifiedURL.searchParams.set(TIMEOUT_PARAM_NAME, '30'); + } + + const abortController = new AbortController(); + + const connection = new NetworkInformationPolyfill(rectifiedURL, { + firstChunkWithin: rectifiedFirstChunkWithin, + shortPollURL: rectifiedShortPollURL, + signal: abortController.signal + }); + + entry = Object.freeze([connection, abortController]); + this.#connections.set(url, entry); + } + + this.#callback(entry[0]); + } + + unobserve(url: string) { + this.#connections.get(url)?.[1].abort(); + this.#connections.delete(url); + } +} diff --git a/src/streaming/mergeAbortSignal.ts b/src/streaming/mergeAbortSignal.ts new file mode 100644 index 00000000..f24971d2 --- /dev/null +++ b/src/streaming/mergeAbortSignal.ts @@ -0,0 +1,19 @@ +// TODO: Add tests +export default function mergeAbortSignal(...signals: AbortSignal[]): AbortSignal { + const abortController = new AbortController(); + let handleAbort: () => void; + + handleAbort = () => { + abortController.abort(); + + for (const signal of signals) { + signal.removeEventListener('abort', handleAbort); + } + }; + + for (const signal of signals) { + signal.addEventListener('abort', handleAbort, { once: true }); + } + + return abortController.signal; +} diff --git a/src/streaming/probeNetworkInformation.ts b/src/streaming/probeNetworkInformation.ts index e7f2779b..345c5af8 100644 --- a/src/streaming/probeNetworkInformation.ts +++ b/src/streaming/probeNetworkInformation.ts @@ -1,23 +1,4 @@ -declare global { - // This is subset of https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation. - interface NetworkInformation extends EventTarget { - addEventListener( - type: 'change', - listener: EventListener | EventListenerObject, - options?: AddEventListenerOptions | boolean - ): void; - - removeEventListener( - type: 'change', - listener: EventListener | EventListenerObject, - options?: AddEventListenerOptions | boolean - ): void; - } - - interface Navigator { - get connection(): NetworkInformation; - } -} +/// type ProbeNetworkInformationInit = { signal: AbortSignal; From 7962ccd254afe8367822d30c2108f3223f75ed24 Mon Sep 17 00:00:00 2001 From: William Wong Date: Sat, 8 Jul 2023 17:21:59 -0700 Subject: [PATCH 15/33] Update test names --- src/streaming/NetworkInformationObserver.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/streaming/NetworkInformationObserver.test.ts b/src/streaming/NetworkInformationObserver.test.ts index c71ff6a4..257a5fff 100644 --- a/src/streaming/NetworkInformationObserver.test.ts +++ b/src/streaming/NetworkInformationObserver.test.ts @@ -355,13 +355,13 @@ describe('A NetworkInformationObserver', () => { describe.only('after 10 seconds', () => { beforeEach(() => jest.advanceTimersByTimeAsync(10_000)); - test('should reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(3))); + test('should have connected 3 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(3))); }); describe.only('after 20 seconds', () => { beforeEach(() => jest.advanceTimersByTimeAsync(20_000)); - test('should reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(4))); + test('should have connected 4 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(4))); }); }); }); From 63ab648f9daf98345f786033cfe3f41540bb65aa Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 12:52:51 -0700 Subject: [PATCH 16/33] Add WebSocketClient implementation --- ...SocketClientWithNetworkInformation.test.ts | 247 ++++++++++++++++++ .../WebSocketClientWithNetworkInformation.ts | 60 +++++ 2 files changed, 307 insertions(+) create mode 100644 src/streaming/WebSocketClientWithNetworkInformation.test.ts create mode 100644 src/streaming/WebSocketClientWithNetworkInformation.ts diff --git a/src/streaming/WebSocketClientWithNetworkInformation.test.ts b/src/streaming/WebSocketClientWithNetworkInformation.test.ts new file mode 100644 index 00000000..666a9fcb --- /dev/null +++ b/src/streaming/WebSocketClientWithNetworkInformation.test.ts @@ -0,0 +1,247 @@ +import type { WebSocketClient } from 'botframework-streaming'; +import type ActualWebSocketClientWithNetworkInformation from './WebSocketClientWithNetworkInformation'; + +// Mocked modules are available across the test file. They cannot be unmocked. +// Thus, they are more-or-less similar to import/require. +jest.mock('botframework-streaming', () => ({ + __esmodule: true, + WebSocketClient: class WebSocketClient { + constructor({ disconnectionHandler, requestHandler, url }: WebSocketClientInit) { + this.#disconnectionHandler = disconnectionHandler; + this.#requestHandler = requestHandler; + this.#url = url; + + // Set up mocks. + this.#connect = jest.fn(() => Promise.resolve()); + this.#disconnect = jest.fn(() => this.#disconnectionHandler?.('disconnect() is called')); + } + + #connect: () => Promise; + #disconnect: () => void; + #disconnectionHandler: WebSocketClientInit['disconnectionHandler']; + #requestHandler: WebSocketClientInit['requestHandler']; + #url: string; + + connect(): Promise { + return this.#connect(); + } + + disconnect(): void { + return this.#disconnect(); + } + + get __test__connect(): () => Promise { + return this.#connect; + } + + get __test__disconnect(): () => void { + return this.#disconnect; + } + + get __test__disconnectionHandler(): WebSocketClientInit['disconnectionHandler'] { + return this.#disconnectionHandler; + } + + get __test__requestHandler(): WebSocketClientInit['requestHandler'] { + return this.#requestHandler; + } + + get __test__url(): WebSocketClientInit['url'] { + return this.#url; + } + } +})); + +class MockNetworkInformation extends EventTarget { + constructor() { + super(); + } + + #type: 'none' | 'unknown' = 'none'; + + get type() { + return this.#type; + } + + setOffline() { + if (this.#type !== 'none') { + this.#type = 'none'; + this.dispatchEvent(new Event('change')); + } + } + + setOnline() { + if (this.#type === 'none') { + this.#type = 'unknown'; + this.dispatchEvent(new Event('change')); + } + } +} + +type WebSocketClientInit = ConstructorParameters[0]; + +const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); +const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; +const url: string = 'wss://dummy/'; + +let client: WebSocketClient; +let connection: MockNetworkInformation; + +beforeEach(() => { + connection = new MockNetworkInformation(); + + let WebSocketClientWithNetworkInformation: typeof ActualWebSocketClientWithNetworkInformation; + + WebSocketClientWithNetworkInformation = require('./WebSocketClientWithNetworkInformation').default; + + client = new WebSocketClientWithNetworkInformation({ + disconnectionHandler, + networkInformationConnection: connection, + requestHandler, + url + }); + + // Spy on all `console.warn()`. + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => jest.restoreAllMocks()); + +describe('constructor', () => { + test('should pass `disconnectionHandler`', () => + expect(client['__test__disconnectionHandler']).toBe(disconnectionHandler)); + test('should pass `requestHandler`', () => expect(client['__test__requestHandler']).toBe(requestHandler)); + test('should pass `url`', () => expect(client['__test__url']).toBe(url)); +}); + +describe('initially online', () => { + beforeEach(() => connection.setOnline()); + + test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); + + describe('when connect() is called', () => { + beforeEach(() => client.connect()); + + test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); + test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); + test('should not call disconnectionHandler', () => + expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); + + describe('when offline', () => { + beforeEach(() => connection.setOffline()); + + test('should call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + + // If connected, it should call disconnectionHandler. + test('should call disconnectionHandler', () => expect(client['__test__disconnectionHandler']).toBeCalledTimes(1)); + + describe('when connect() is called after disconnect', () => { + let promise; + + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + promise = client.connect(); + }); + + test('should resolve', () => expect(promise).resolves.toBeUndefined()); + test('should warn', () => { + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); + }); + }); + }); + + describe('when connect() is called twice', () => { + let promise; + + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + promise = client.connect(); + }); + + test('should resolve', () => expect(promise).resolves.toBeUndefined()); + test('should warn', () => { + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); + }); + }); + }); +}); + +describe('initially offline', () => { + test('NetworkInformation should have type of "none"', () => expect(connection).toHaveProperty('type', 'none')); + + describe('when connect() is called', () => { + let promise; + + beforeEach(() => { + promise = client.connect(); + promise.catch(() => {}); + }); + + test('should throw', () => expect(() => promise).rejects.toThrow()); + + // If never connected, it did not need to call disconnectionHandler. + test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); + test('should not call disconnectionHandler', () => + expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); + }); +}); + +// describe('initially online', () => { +// beforeEach(() => connection.setOnline()); + +// describe('connect()', () => { +// test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); + +// describe('when called', () => { +// beforeEach(() => client.connect()); + +// test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); + +// describe('twice', () => { +// beforeEach(() => client.connect()); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connect() can only be called once"', () => +// expect(console.warn).toHaveBeenNthCalledWith( +// 1, +// expect.stringContaining('connect() can only be called once') +// )); +// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); +// }); + +// describe('then abort the probe', () => { +// beforeEach(() => connection.setOffline()); + +// test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + +// describe('when connect() is called again', () => { +// beforeEach(() => client.connect()); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connect() can only be called once"', () => +// expect(console.warn).toHaveBeenNthCalledWith( +// 1, +// expect.stringContaining('connect() can only be called once') +// )); +// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); +// }); +// }); +// }); +// }); +// }); + +// describe('connection is offline then call connect()', () => { +// beforeEach(() => { +// connection.setOffline(); +// client.connect(); +// }); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connection is offline before connect()"', () => +// expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); +// test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); +// }); diff --git a/src/streaming/WebSocketClientWithNetworkInformation.ts b/src/streaming/WebSocketClientWithNetworkInformation.ts new file mode 100644 index 00000000..aed36709 --- /dev/null +++ b/src/streaming/WebSocketClientWithNetworkInformation.ts @@ -0,0 +1,60 @@ +import { WebSocketClient } from 'botframework-streaming'; + +import type { RequestHandler } from 'botframework-streaming'; + +type WebSocketClientWithNetworkInformationInit = { + /** + * Gets or sets the observer function for disconnection or error sending/receiving through WebSocket. + * + * Note: This function could be called multiple times, the callee is expected to ignore subsequent calls. + */ + disconnectionHandler: (message: string) => void; + networkInformationConnection: NetworkInformation; + requestHandler: RequestHandler; + url: string; +}; + +export default class WebSocketClientWithNetworkInformation extends WebSocketClient { + constructor({ + disconnectionHandler, + networkInformationConnection, + requestHandler, + url + }: WebSocketClientWithNetworkInformationInit) { + super({ + disconnectionHandler, + requestHandler, + url + }); + + this.#networkInformationConnection = networkInformationConnection; + } + + #connectCalled: boolean = false; + #networkInformationConnection: NetworkInformation; + + // TODO: Better, the `NetworkInformation` instance should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. + // `BrowserWebSocket` is where it creates `WebSocket` object. + // The `NetworkInformation` instance should accompany `WebSocket` and forcibly close it on abort. + // Maybe `botframework-streaming` should accept ponyfills. + connect(): Promise { + if (this.#connectCalled) { + console.warn('botframework-directlinejs: connect() can only be called once.'); + + return Promise.resolve(); + } + + this.#connectCalled = true; + + if (this.#networkInformationConnection.type === 'none') { + return Promise.reject(new Error('botframework-directlinejs: Failed to connect while offline.')); + } + + this.#networkInformationConnection.addEventListener( + 'change', + () => this.#networkInformationConnection.type === 'none' && this.disconnect() + ); + + return super.connect(); + } +} From 125f8a4f2b707d562beee9b1257e6d6e47771a34 Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 13:40:25 -0700 Subject: [PATCH 17/33] Add tests --- ....ts => NetworkInformationObserver.spec.ts} | 0 src/streaming/NetworkInformationObserver.ts | 8 +- ...SocketClientWithNetworkInformation.spec.ts | 247 ++++++++++++++++++ ...SocketClientWithNetworkInformation.test.ts | 200 +++----------- src/streaming/mergeAbortSignal.spec.ts | 36 +++ .../{sleep.test.ts => sleep.spec.ts} | 0 6 files changed, 318 insertions(+), 173 deletions(-) rename src/streaming/{NetworkInformationObserver.test.ts => NetworkInformationObserver.spec.ts} (100%) create mode 100644 src/streaming/WebSocketClientWithNetworkInformation.spec.ts create mode 100644 src/streaming/mergeAbortSignal.spec.ts rename src/streaming/{sleep.test.ts => sleep.spec.ts} (100%) diff --git a/src/streaming/NetworkInformationObserver.test.ts b/src/streaming/NetworkInformationObserver.spec.ts similarity index 100% rename from src/streaming/NetworkInformationObserver.test.ts rename to src/streaming/NetworkInformationObserver.spec.ts diff --git a/src/streaming/NetworkInformationObserver.ts b/src/streaming/NetworkInformationObserver.ts index 2848ce76..9a5b1ea1 100644 --- a/src/streaming/NetworkInformationObserver.ts +++ b/src/streaming/NetworkInformationObserver.ts @@ -33,11 +33,11 @@ class NetworkInformationPolyfill extends EventTarget implements NetworkInformati return this.#type; } - #setOffline() { + #goOffline() { this.#setType('none'); } - #setOnline() { + #goOnline() { this.#setType('unknown'); } @@ -69,7 +69,7 @@ class NetworkInformationPolyfill extends EventTarget implements NetworkInformati ]); // Received first chunk. - this.#setOnline(); + this.#goOnline(); await res.arrayBuffer(); const timeToSleep = Math.max(0, MINIMUM_PING_INTERVAL + startTime - Date.now()); @@ -89,7 +89,7 @@ class NetworkInformationPolyfill extends EventTarget implements NetworkInformati } catch (error) { currentAbortController.abort(); - this.#setOffline(); + this.#goOffline(); shortPing = true; await sleep(SLEEP_INTERVAL_AFTER_ERROR, { signal: this.#signal }); diff --git a/src/streaming/WebSocketClientWithNetworkInformation.spec.ts b/src/streaming/WebSocketClientWithNetworkInformation.spec.ts new file mode 100644 index 00000000..666a9fcb --- /dev/null +++ b/src/streaming/WebSocketClientWithNetworkInformation.spec.ts @@ -0,0 +1,247 @@ +import type { WebSocketClient } from 'botframework-streaming'; +import type ActualWebSocketClientWithNetworkInformation from './WebSocketClientWithNetworkInformation'; + +// Mocked modules are available across the test file. They cannot be unmocked. +// Thus, they are more-or-less similar to import/require. +jest.mock('botframework-streaming', () => ({ + __esmodule: true, + WebSocketClient: class WebSocketClient { + constructor({ disconnectionHandler, requestHandler, url }: WebSocketClientInit) { + this.#disconnectionHandler = disconnectionHandler; + this.#requestHandler = requestHandler; + this.#url = url; + + // Set up mocks. + this.#connect = jest.fn(() => Promise.resolve()); + this.#disconnect = jest.fn(() => this.#disconnectionHandler?.('disconnect() is called')); + } + + #connect: () => Promise; + #disconnect: () => void; + #disconnectionHandler: WebSocketClientInit['disconnectionHandler']; + #requestHandler: WebSocketClientInit['requestHandler']; + #url: string; + + connect(): Promise { + return this.#connect(); + } + + disconnect(): void { + return this.#disconnect(); + } + + get __test__connect(): () => Promise { + return this.#connect; + } + + get __test__disconnect(): () => void { + return this.#disconnect; + } + + get __test__disconnectionHandler(): WebSocketClientInit['disconnectionHandler'] { + return this.#disconnectionHandler; + } + + get __test__requestHandler(): WebSocketClientInit['requestHandler'] { + return this.#requestHandler; + } + + get __test__url(): WebSocketClientInit['url'] { + return this.#url; + } + } +})); + +class MockNetworkInformation extends EventTarget { + constructor() { + super(); + } + + #type: 'none' | 'unknown' = 'none'; + + get type() { + return this.#type; + } + + setOffline() { + if (this.#type !== 'none') { + this.#type = 'none'; + this.dispatchEvent(new Event('change')); + } + } + + setOnline() { + if (this.#type === 'none') { + this.#type = 'unknown'; + this.dispatchEvent(new Event('change')); + } + } +} + +type WebSocketClientInit = ConstructorParameters[0]; + +const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); +const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; +const url: string = 'wss://dummy/'; + +let client: WebSocketClient; +let connection: MockNetworkInformation; + +beforeEach(() => { + connection = new MockNetworkInformation(); + + let WebSocketClientWithNetworkInformation: typeof ActualWebSocketClientWithNetworkInformation; + + WebSocketClientWithNetworkInformation = require('./WebSocketClientWithNetworkInformation').default; + + client = new WebSocketClientWithNetworkInformation({ + disconnectionHandler, + networkInformationConnection: connection, + requestHandler, + url + }); + + // Spy on all `console.warn()`. + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => jest.restoreAllMocks()); + +describe('constructor', () => { + test('should pass `disconnectionHandler`', () => + expect(client['__test__disconnectionHandler']).toBe(disconnectionHandler)); + test('should pass `requestHandler`', () => expect(client['__test__requestHandler']).toBe(requestHandler)); + test('should pass `url`', () => expect(client['__test__url']).toBe(url)); +}); + +describe('initially online', () => { + beforeEach(() => connection.setOnline()); + + test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); + + describe('when connect() is called', () => { + beforeEach(() => client.connect()); + + test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); + test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); + test('should not call disconnectionHandler', () => + expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); + + describe('when offline', () => { + beforeEach(() => connection.setOffline()); + + test('should call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + + // If connected, it should call disconnectionHandler. + test('should call disconnectionHandler', () => expect(client['__test__disconnectionHandler']).toBeCalledTimes(1)); + + describe('when connect() is called after disconnect', () => { + let promise; + + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + promise = client.connect(); + }); + + test('should resolve', () => expect(promise).resolves.toBeUndefined()); + test('should warn', () => { + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); + }); + }); + }); + + describe('when connect() is called twice', () => { + let promise; + + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + + promise = client.connect(); + }); + + test('should resolve', () => expect(promise).resolves.toBeUndefined()); + test('should warn', () => { + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); + }); + }); + }); +}); + +describe('initially offline', () => { + test('NetworkInformation should have type of "none"', () => expect(connection).toHaveProperty('type', 'none')); + + describe('when connect() is called', () => { + let promise; + + beforeEach(() => { + promise = client.connect(); + promise.catch(() => {}); + }); + + test('should throw', () => expect(() => promise).rejects.toThrow()); + + // If never connected, it did not need to call disconnectionHandler. + test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); + test('should not call disconnectionHandler', () => + expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); + }); +}); + +// describe('initially online', () => { +// beforeEach(() => connection.setOnline()); + +// describe('connect()', () => { +// test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); + +// describe('when called', () => { +// beforeEach(() => client.connect()); + +// test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); + +// describe('twice', () => { +// beforeEach(() => client.connect()); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connect() can only be called once"', () => +// expect(console.warn).toHaveBeenNthCalledWith( +// 1, +// expect.stringContaining('connect() can only be called once') +// )); +// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); +// }); + +// describe('then abort the probe', () => { +// beforeEach(() => connection.setOffline()); + +// test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + +// describe('when connect() is called again', () => { +// beforeEach(() => client.connect()); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connect() can only be called once"', () => +// expect(console.warn).toHaveBeenNthCalledWith( +// 1, +// expect.stringContaining('connect() can only be called once') +// )); +// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); +// }); +// }); +// }); +// }); +// }); + +// describe('connection is offline then call connect()', () => { +// beforeEach(() => { +// connection.setOffline(); +// client.connect(); +// }); + +// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); +// test('should warn "connection is offline before connect()"', () => +// expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); +// test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); +// }); diff --git a/src/streaming/WebSocketClientWithNetworkInformation.test.ts b/src/streaming/WebSocketClientWithNetworkInformation.test.ts index 666a9fcb..59914b8b 100644 --- a/src/streaming/WebSocketClientWithNetworkInformation.test.ts +++ b/src/streaming/WebSocketClientWithNetworkInformation.test.ts @@ -1,54 +1,15 @@ import type { WebSocketClient } from 'botframework-streaming'; -import type ActualWebSocketClientWithNetworkInformation from './WebSocketClientWithNetworkInformation'; +import type OriginalWebSocketClientWithNetworkInformation from './WebSocketClientWithNetworkInformation'; // Mocked modules are available across the test file. They cannot be unmocked. // Thus, they are more-or-less similar to import/require. -jest.mock('botframework-streaming', () => ({ +jest.mock('../../node_modules/botframework-streaming/lib/webSocket/nodeWebSocket', () => ({ __esmodule: true, - WebSocketClient: class WebSocketClient { - constructor({ disconnectionHandler, requestHandler, url }: WebSocketClientInit) { - this.#disconnectionHandler = disconnectionHandler; - this.#requestHandler = requestHandler; - this.#url = url; - - // Set up mocks. - this.#connect = jest.fn(() => Promise.resolve()); - this.#disconnect = jest.fn(() => this.#disconnectionHandler?.('disconnect() is called')); - } - - #connect: () => Promise; - #disconnect: () => void; - #disconnectionHandler: WebSocketClientInit['disconnectionHandler']; - #requestHandler: WebSocketClientInit['requestHandler']; - #url: string; - - connect(): Promise { - return this.#connect(); - } - - disconnect(): void { - return this.#disconnect(); - } - - get __test__connect(): () => Promise { - return this.#connect; - } - - get __test__disconnect(): () => void { - return this.#disconnect; - } - - get __test__disconnectionHandler(): WebSocketClientInit['disconnectionHandler'] { - return this.#disconnectionHandler; - } - - get __test__requestHandler(): WebSocketClientInit['requestHandler'] { - return this.#requestHandler; - } - - get __test__url(): WebSocketClientInit['url'] { - return this.#url; - } + NodeWebSocket: class { + connect() {} + setOnCloseHandler() {} + setOnErrorHandler() {} + setOnMessageHandler() {} } })); @@ -85,18 +46,18 @@ const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: const url: string = 'wss://dummy/'; let client: WebSocketClient; -let connection: MockNetworkInformation; +let networkInformationConnection: MockNetworkInformation; beforeEach(() => { - connection = new MockNetworkInformation(); + networkInformationConnection = new MockNetworkInformation(); - let WebSocketClientWithNetworkInformation: typeof ActualWebSocketClientWithNetworkInformation; + let WebSocketClientWithNetworkInformation: typeof OriginalWebSocketClientWithNetworkInformation; WebSocketClientWithNetworkInformation = require('./WebSocketClientWithNetworkInformation').default; client = new WebSocketClientWithNetworkInformation({ disconnectionHandler, - networkInformationConnection: connection, + networkInformationConnection, requestHandler, url }); @@ -107,141 +68,42 @@ beforeEach(() => { afterEach(() => jest.restoreAllMocks()); -describe('constructor', () => { - test('should pass `disconnectionHandler`', () => - expect(client['__test__disconnectionHandler']).toBe(disconnectionHandler)); - test('should pass `requestHandler`', () => expect(client['__test__requestHandler']).toBe(requestHandler)); - test('should pass `url`', () => expect(client['__test__url']).toBe(url)); -}); +test('should not call disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(0)); describe('initially online', () => { - beforeEach(() => connection.setOnline()); - - test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); + beforeEach(() => networkInformationConnection.setOnline()); describe('when connect() is called', () => { beforeEach(() => client.connect()); - test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); - test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); - test('should not call disconnectionHandler', () => - expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); + describe('call disconnect()', () => { + beforeEach(() => client.disconnect()); - describe('when offline', () => { - beforeEach(() => connection.setOffline()); - - test('should call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); - - // If connected, it should call disconnectionHandler. - test('should call disconnectionHandler', () => expect(client['__test__disconnectionHandler']).toBeCalledTimes(1)); - - describe('when connect() is called after disconnect', () => { - let promise; - - beforeEach(() => { - jest.spyOn(console, 'warn').mockImplementation(() => {}); + // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. + test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); - promise = client.connect(); - }); + describe('when offline', () => { + beforeEach(() => networkInformationConnection.setOffline()); - test('should resolve', () => expect(promise).resolves.toBeUndefined()); - test('should warn', () => { - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); - }); + // After disconnected() is called, there should be no extra calls for offline. + test('should have no extra calls to disconnectHandler()', () => + expect(disconnectionHandler).toBeCalledTimes(2)); }); }); - describe('when connect() is called twice', () => { - let promise; + describe('when offline', () => { + beforeEach(() => networkInformationConnection.setOffline()); - beforeEach(() => { - jest.spyOn(console, 'warn').mockImplementation(() => {}); + // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. + test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); - promise = client.connect(); - }); + describe('when disconnect() is called', () => { + beforeEach(() => client.disconnect()); - test('should resolve', () => expect(promise).resolves.toBeUndefined()); - test('should warn', () => { - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once')); + // After the signal is aborted, there should be no extra calls for calling disconnect(). + test('should have no extra calls to disconnectHandler()', () => + expect(disconnectionHandler).toBeCalledTimes(2)); }); }); }); }); - -describe('initially offline', () => { - test('NetworkInformation should have type of "none"', () => expect(connection).toHaveProperty('type', 'none')); - - describe('when connect() is called', () => { - let promise; - - beforeEach(() => { - promise = client.connect(); - promise.catch(() => {}); - }); - - test('should throw', () => expect(() => promise).rejects.toThrow()); - - // If never connected, it did not need to call disconnectionHandler. - test('should not call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(0)); - test('should not call disconnectionHandler', () => - expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); - }); -}); - -// describe('initially online', () => { -// beforeEach(() => connection.setOnline()); - -// describe('connect()', () => { -// test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); - -// describe('when called', () => { -// beforeEach(() => client.connect()); - -// test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); - -// describe('twice', () => { -// beforeEach(() => client.connect()); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connect() can only be called once"', () => -// expect(console.warn).toHaveBeenNthCalledWith( -// 1, -// expect.stringContaining('connect() can only be called once') -// )); -// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); -// }); - -// describe('then abort the probe', () => { -// beforeEach(() => connection.setOffline()); - -// test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); - -// describe('when connect() is called again', () => { -// beforeEach(() => client.connect()); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connect() can only be called once"', () => -// expect(console.warn).toHaveBeenNthCalledWith( -// 1, -// expect.stringContaining('connect() can only be called once') -// )); -// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); -// }); -// }); -// }); -// }); -// }); - -// describe('connection is offline then call connect()', () => { -// beforeEach(() => { -// connection.setOffline(); -// client.connect(); -// }); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connection is offline before connect()"', () => -// expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); -// test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); -// }); diff --git a/src/streaming/mergeAbortSignal.spec.ts b/src/streaming/mergeAbortSignal.spec.ts new file mode 100644 index 00000000..403e1aff --- /dev/null +++ b/src/streaming/mergeAbortSignal.spec.ts @@ -0,0 +1,36 @@ +import mergeAbortSignal from './mergeAbortSignal'; + +describe('merging 3 abort signals', () => { + let abortControllers: AbortController[]; + let abortSignal: AbortSignal; + let handleAbort: jest.Mock; + + beforeEach(() => { + abortControllers = new Array(3).fill(undefined).map(() => new AbortController()); + handleAbort = jest.fn(); + + abortSignal = mergeAbortSignal(...abortControllers.map(({ signal }) => signal)); + abortSignal.addEventListener('abort', handleAbort); + }); + + test('should not abort initially', () => expect(abortSignal).toHaveProperty('aborted', false)); + test('should not receive "abort" event initially', () => expect(handleAbort).toBeCalledTimes(0)); + + describe.each([ + ['first', 0], + ['second', 1], + ['third', 2] + ])('when %s signal is aborted', (_, index) => { + beforeEach(() => abortControllers[index].abort()); + + test('should abort the signal', () => expect(abortSignal).toHaveProperty('aborted', true)); + test('should receive "abort" event', () => expect(handleAbort).toBeCalledTimes(1)); + + describe('when all signals are aborted', () => { + beforeEach(() => abortControllers.forEach(abortController => abortController.abort())); + + test('should abort the signal', () => expect(abortSignal).toHaveProperty('aborted', true)); + test('should receive "abort" event once', () => expect(handleAbort).toBeCalledTimes(1)); + }); + }); +}); diff --git a/src/streaming/sleep.test.ts b/src/streaming/sleep.spec.ts similarity index 100% rename from src/streaming/sleep.test.ts rename to src/streaming/sleep.spec.ts From fcd2e36d24bb185e8a3fa6f5d4dacbabc0c2726e Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 15:51:06 -0700 Subject: [PATCH 18/33] Add test for change network type --- ...SocketClientWithNetworkInformation.spec.ts | 92 +++++-------------- 1 file changed, 22 insertions(+), 70 deletions(-) diff --git a/src/streaming/WebSocketClientWithNetworkInformation.spec.ts b/src/streaming/WebSocketClientWithNetworkInformation.spec.ts index 666a9fcb..da6b8172 100644 --- a/src/streaming/WebSocketClientWithNetworkInformation.spec.ts +++ b/src/streaming/WebSocketClientWithNetworkInformation.spec.ts @@ -52,27 +52,22 @@ jest.mock('botframework-streaming', () => ({ } })); +type NetworkInformationType = 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; + class MockNetworkInformation extends EventTarget { constructor() { super(); } - #type: 'none' | 'unknown' = 'none'; + #type: NetworkInformationType = 'none'; get type() { return this.#type; } - setOffline() { - if (this.#type !== 'none') { - this.#type = 'none'; - this.dispatchEvent(new Event('change')); - } - } - - setOnline() { - if (this.#type === 'none') { - this.#type = 'unknown'; + set type(value: NetworkInformationType) { + if (this.#type !== value) { + this.#type = value; this.dispatchEvent(new Event('change')); } } @@ -96,7 +91,7 @@ beforeEach(() => { client = new WebSocketClientWithNetworkInformation({ disconnectionHandler, - networkInformationConnection: connection, + networkInformation: connection, requestHandler, url }); @@ -115,7 +110,9 @@ describe('constructor', () => { }); describe('initially online', () => { - beforeEach(() => connection.setOnline()); + beforeEach(() => { + connection.type = 'wifi'; + }); test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); @@ -128,7 +125,9 @@ describe('initially online', () => { expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); describe('when offline', () => { - beforeEach(() => connection.setOffline()); + beforeEach(() => { + connection.type = 'none'; + }); test('should call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); @@ -152,6 +151,15 @@ describe('initially online', () => { }); }); + describe('when network type change to "bluetooth"', () => { + beforeEach(() => { + connection.type = 'bluetooth'; + }); + + test('should call super.disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); + test('should call disconnectionHandler', () => expect(client['__test__disconnectionHandler']).toBeCalledTimes(1)); + }); + describe('when connect() is called twice', () => { let promise; @@ -189,59 +197,3 @@ describe('initially offline', () => { expect(client['__test__disconnectionHandler']).toBeCalledTimes(0)); }); }); - -// describe('initially online', () => { -// beforeEach(() => connection.setOnline()); - -// describe('connect()', () => { -// test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); - -// describe('when called', () => { -// beforeEach(() => client.connect()); - -// test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); - -// describe('twice', () => { -// beforeEach(() => client.connect()); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connect() can only be called once"', () => -// expect(console.warn).toHaveBeenNthCalledWith( -// 1, -// expect.stringContaining('connect() can only be called once') -// )); -// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); -// }); - -// describe('then abort the probe', () => { -// beforeEach(() => connection.setOffline()); - -// test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); - -// describe('when connect() is called again', () => { -// beforeEach(() => client.connect()); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connect() can only be called once"', () => -// expect(console.warn).toHaveBeenNthCalledWith( -// 1, -// expect.stringContaining('connect() can only be called once') -// )); -// test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); -// }); -// }); -// }); -// }); -// }); - -// describe('connection is offline then call connect()', () => { -// beforeEach(() => { -// connection.setOffline(); -// client.connect(); -// }); - -// test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); -// test('should warn "connection is offline before connect()"', () => -// expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); -// test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); -// }); From ea2437f3817698e9f637bea7a13cd045bfec9ecc Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:04:39 -0700 Subject: [PATCH 19/33] Move to NetworkInformation --- .../networkProbe.function.story.ts | 147 ----------- .../networkProbe.rest.story.ts | 130 ---------- ...ts => options.networkInformation.story.ts} | 19 +- src/directLineStreaming.ts | 88 ++----- .../NetworkInformationObserver.spec.ts | 4 +- ...SocketClientWithNetworkInformation.test.ts | 31 +-- .../WebSocketClientWithNetworkInformation.ts | 37 ++- ...bSocketClientWithProbe.integration.test.ts | 77 ------ .../WebSocketClientWithProbe.test.ts | 138 ----------- src/streaming/WebSocketClientWithProbe.ts | 55 ----- src/streaming/probeNetworkInformation.test.ts | 25 -- src/streaming/probeNetworkInformation.ts | 27 -- src/streaming/probeREST.test.ts | 230 ------------------ src/streaming/probeREST.ts | 98 -------- 14 files changed, 81 insertions(+), 1025 deletions(-) delete mode 100644 __tests__/directLineStreaming/networkProbe.function.story.ts delete mode 100644 __tests__/directLineStreaming/networkProbe.rest.story.ts rename __tests__/directLineStreaming/{networkProbe.networkInformation.story.ts => options.networkInformation.story.ts} (90%) delete mode 100644 src/streaming/WebSocketClientWithProbe.integration.test.ts delete mode 100644 src/streaming/WebSocketClientWithProbe.test.ts delete mode 100644 src/streaming/WebSocketClientWithProbe.ts delete mode 100644 src/streaming/probeNetworkInformation.test.ts delete mode 100644 src/streaming/probeNetworkInformation.ts delete mode 100644 src/streaming/probeREST.test.ts delete mode 100644 src/streaming/probeREST.ts diff --git a/__tests__/directLineStreaming/networkProbe.function.story.ts b/__tests__/directLineStreaming/networkProbe.function.story.ts deleted file mode 100644 index 41bdda9c..00000000 --- a/__tests__/directLineStreaming/networkProbe.function.story.ts +++ /dev/null @@ -1,147 +0,0 @@ -/// - -import fetch from 'node-fetch'; - -import { ConnectionStatus } from '../../src/directLine'; -import { DirectLineStreaming } from '../../src/directLineStreaming'; -import mockObserver from './__setup__/mockObserver'; -import setupBotProxy from './__setup__/setupBotProxy'; -import waitFor from './__setup__/external/testing-library/waitFor'; - -type MockObserver = ReturnType; -type ResultOfPromise = T extends PromiseLike ? P : never; - -const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; -const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; - -jest.setTimeout(10_000); - -// GIVEN: A Direct Line Streaming chat adapter with network probe on REST API. -describe('Direct Line Streaming chat adapter with network probe on REST API', () => { - let activityObserver: MockObserver; - let botProxy: ResultOfPromise>; - let connectionStatusObserver: MockObserver; - let createAbortController: jest.Mock; - let directLine: DirectLineStreaming; - - beforeEach(async () => { - jest.useFakeTimers({ now: 0 }); - - let token: string; - - [botProxy, { token }] = await Promise.all([ - setupBotProxy({ streamingBotURL: MOCKBOT3_URL }), - fetch(TOKEN_URL, { method: 'POST' }).then(res => res.json()) - ]); - - createAbortController = jest.fn(({ signal }) => { - const abortController = new AbortController(); - - signal.addEventListener('abort', () => abortController.abort(), { once: true }); - - return abortController; - }); - - activityObserver = mockObserver(); - connectionStatusObserver = mockObserver(); - directLine = new DirectLineStreaming({ - domain: botProxy.directLineStreamingURL, - networkProbe: ({ signal }) => createAbortController({ signal }).signal, - token - }); - - directLine.connectionStatus$.subscribe(connectionStatusObserver); - }); - - afterEach(() => { - directLine.end(); - - jest.useRealTimers(); - }); - - describe('when connect', () => { - // WHEN: Connect. - beforeEach(() => directLine.activity$.subscribe(activityObserver)); - - // THEN: Should observe "Uninitialized" -> "Connecting" -> "Online". - test('should observe "Uninitialized" -> "Connecting" -> "Online"', () => - waitFor( - () => - expect(connectionStatusObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', ConnectionStatus.Uninitialized], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online] - ]), - { timeout: 5_000 } - )); - - // WHEN: Connection status become "Online" and network probe is created. - describe('after online and network probe is connected', () => { - beforeEach(() => - waitFor( - () => { - expect(connectionStatusObserver.observe).toHaveBeenLastCalledWith([ - expect.any(Number), - 'next', - ConnectionStatus.Online - ]); - - expect(createAbortController).toBeCalledTimes(1); - }, - { timeout: 5_000 } - ) - ); - - // THEN: Should receive "Hello and welcome!" - test('should receive "Hello and welcome!"', () => - waitFor( - () => - expect(activityObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', expect.activityContaining('Hello and welcome!')] - ]), - { timeout: 5_000 } - )); - - // WHEN: Network probing connection is closed. - describe('when the probing connection detected a fault', () => { - beforeEach(() => { - const { - mock: { - results: [firstResult] - } - } = createAbortController; - - expect(firstResult).toHaveProperty('type', 'return'); - - firstResult.value.abort(); - }); - - // THEN: Should observe "Connecting" -> "Online" again. - test('should observe ... -> "Connecting" -> "Online"', () => - waitFor( - () => - expect(connectionStatusObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', ConnectionStatus.Uninitialized], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online] - ]), - { timeout: 5_000 } - )); - - test('should recreate network probe', () => - waitFor(() => expect(createAbortController).toBeCalledTimes(2), { timeout: 5_000 })); - }); - - describe('when connection is closed', () => { - beforeEach(() => directLine.end()); - - test('should abort the network probe', () => { - expect(createAbortController).toHaveBeenCalledTimes(1); - expect(createAbortController.mock.calls[0][0]).toHaveProperty('signal.aborted', true); - }); - }); - }); - }); -}); diff --git a/__tests__/directLineStreaming/networkProbe.rest.story.ts b/__tests__/directLineStreaming/networkProbe.rest.story.ts deleted file mode 100644 index 8edabae3..00000000 --- a/__tests__/directLineStreaming/networkProbe.rest.story.ts +++ /dev/null @@ -1,130 +0,0 @@ -/// - -import fetch from 'node-fetch'; - -import { ConnectionStatus } from '../../src/directLine'; -import { DirectLineStreaming } from '../../src/directLineStreaming'; -import mockObserver from './__setup__/mockObserver'; -import setupBotProxy from './__setup__/setupBotProxy'; -import waitFor from './__setup__/external/testing-library/waitFor'; - -type MockObserver = ReturnType; -type ResultOfPromise = T extends PromiseLike ? P : never; - -const MOCKBOT3_URL = 'https://webchat-mockbot3.azurewebsites.net/'; -const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directlinease'; - -jest.setTimeout(10_000); - -// GIVEN: A Direct Line Streaming chat adapter with network probe on REST API. -describe('Direct Line Streaming chat adapter with network probe on REST API', () => { - let activityObserver: MockObserver; - let botProxy: ResultOfPromise>; - let connectionStatusObserver: MockObserver; - let directLine: DirectLineStreaming; - - beforeEach(async () => { - jest.useFakeTimers({ now: 0 }); - - let token: string; - - [botProxy, { token }] = await Promise.all([ - setupBotProxy({ streamingBotURL: MOCKBOT3_URL }), - fetch(TOKEN_URL, { method: 'POST' }).then(res => res.json()) - ]); - - activityObserver = mockObserver(); - connectionStatusObserver = mockObserver(); - directLine = new DirectLineStreaming({ - domain: botProxy.directLineStreamingURL, - networkProbe: { url: botProxy.networkProbeURL }, - token - }); - - directLine.connectionStatus$.subscribe(connectionStatusObserver); - }); - - afterEach(() => { - directLine.end(); - - jest.useRealTimers(); - }); - - describe('when connect', () => { - // WHEN: Connect. - beforeEach(() => directLine.activity$.subscribe(activityObserver)); - - // THEN: Should observe "Uninitialized" -> "Connecting" -> "Online". - test('should observe "Uninitialized" -> "Connecting" -> "Online"', () => - waitFor( - () => - expect(connectionStatusObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', ConnectionStatus.Uninitialized], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online] - ]), - { timeout: 5_000 } - )); - - // WHEN: Connection status become "Online" and the network probe is connected. - describe('after online and the network probe is connected', () => { - beforeEach(() => - waitFor( - () => { - expect(connectionStatusObserver.observe).toHaveBeenLastCalledWith([ - expect.any(Number), - 'next', - ConnectionStatus.Online - ]); - - expect(botProxy.numNetworkProbingConnection).toBe(1); - expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(1); - }, - { timeout: 5_000 } - ) - ); - - // THEN: Should receive "Hello and welcome!" - test('should receive "Hello and welcome!"', () => - waitFor( - () => - expect(activityObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', expect.activityContaining('Hello and welcome!')] - ]), - { timeout: 5_000 } - )); - - // WHEN: The network probing connection is forcibly closed. - describe('when the network probing connection is forcibly closed', () => { - beforeEach(() => botProxy.closeAllNetworkProbingConnections()); - - // THEN: Should observe "Connecting" -> "Online" again. - test('should observe ... -> "Connecting" -> "Online"', () => - waitFor( - () => - expect(connectionStatusObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', ConnectionStatus.Uninitialized], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online] - ]), - { timeout: 5_000 } - )); - - test('should reconnect the network probe', () => - waitFor(() => expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(2), { timeout: 5_000 })); - }); - - describe('when the chat adapter is closed', () => { - beforeEach(() => directLine.end()); - - test('should close the network probing connection', () => - waitFor(() => { - expect(botProxy.numOverTheLifetimeNetworkProbingConnection).toBe(1); - expect(botProxy.numNetworkProbingConnection).toBe(0); - })); - }); - }); - }); -}); diff --git a/__tests__/directLineStreaming/networkProbe.networkInformation.story.ts b/__tests__/directLineStreaming/options.networkInformation.story.ts similarity index 90% rename from __tests__/directLineStreaming/networkProbe.networkInformation.story.ts rename to __tests__/directLineStreaming/options.networkInformation.story.ts index 78bc874c..2200d5f0 100644 --- a/__tests__/directLineStreaming/networkProbe.networkInformation.story.ts +++ b/__tests__/directLineStreaming/options.networkInformation.story.ts @@ -27,6 +27,19 @@ describe('Direct Line Streaming chat adapter with network probe on Network Infor jest.useFakeTimers({ now: 0 }); const networkInformation = new EventTarget(); + let type: string = 'wifi'; + + Object.defineProperty(networkInformation, 'type', { + get() { + return type; + }, + set(value: string) { + if (type !== value) { + type = value; + networkInformation.dispatchEvent(new Event('change')); + } + } + }); (global as any).navigator = { get connection() { @@ -45,7 +58,7 @@ describe('Direct Line Streaming chat adapter with network probe on Network Infor connectionStatusObserver = mockObserver(); directLine = new DirectLineStreaming({ domain: botProxy.directLineStreamingURL, - networkProbe: navigator.connection, + networkInformation: navigator.connection, token }); @@ -100,7 +113,9 @@ describe('Direct Line Streaming chat adapter with network probe on Network Infor // WHEN: "change" event is received. describe('when "change" event is received', () => { - beforeEach(() => navigator.connection.dispatchEvent(new Event('change'))); + beforeEach(() => { + (navigator.connection as any).type = 'bluetooth'; + }); // THEN: Should observe "Connecting" -> "Online" again. test('should observe ... -> "Connecting" -> "Online"', () => diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index d1e8bd58..da107cd6 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -8,12 +8,9 @@ import * as BFSE from 'botframework-streaming'; import createDeferred from './createDeferred'; import fetch from 'cross-fetch'; -import probeNetworkInformation from './streaming/probeNetworkInformation'; -import probeREST from './streaming/probeREST'; -import WebSocketClientWithProbe from './streaming/WebSocketClientWithProbe'; - +import { Activity, ConnectionStatus, Conversation, DirectLine, IBotConnection, Media, Message } from './directLine'; +import WebSocketClientWithNetworkInformation from './streaming/WebSocketClientWithNetworkInformation'; import type { Deferred } from './createDeferred'; -import { Activity, ConnectionStatus, Conversation, IBotConnection, Media, Message } from './directLine'; const DIRECT_LINE_VERSION = 'DirectLine/3.0'; const MAX_RETRY_COUNT = 3; @@ -30,42 +27,10 @@ interface DirectLineStreamingOptions { botAgent?: string; /** - * Sets the network probe for assisting detection of connection issues. - * - * When the probe detects any connection issues, the bot connection will be closed and treated as an error. - * - * The probe is intended to assist `WebSocket`. Some implementations of `WebSocket` does not emit `error` event timely in case of connection issues. - * This probe will help declaring connection outages sooner. For example, on iOS/iPadOS 15 and up, the newer "NSURLSession WebSocket" did not signal error on network change. - * - * There are 3 ways to set the probe: `NetworkInformation` instance (using [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API)), `object` (REST API probe), or `function`. - * - * When the probe is an instance of `NetworkInformation`: - * - * - When `change` event is received on the instance, the probe will treat it as a fault. - * - * When the probe is an object, it will probe the liveness of a long-polling service via HTTP GET: - * - * - `url` is the URL of a HTTP GET long-polling service. The service must keep the connection for a period of time and returns HTTP 2xx when the time has passed. - * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends the connection should be kept for 30 seconds. - * - If a non-2xx status code is received, it will be treated as a fault. - * - `minimumInterval` is the time, in millisecond, to wait between pings. Minimum is 10 seconds, default to 25 seconds. - * The interval should be shorter than the time the long-polling API would keep the connection. - * - * When the probe is a function: - * - * - The function will be called when a connection is being established. Thus, a new network probe is needed. - * - The returned `AbortSignal` should be aborted as soon as the probe detects any connection issues. - * - The function should create a new probe on every call and probe should not be reused. - * - When a probe is no longer needed, the `AbortSignal` passed to the function will signal release of underlying resources. - * - At any point of time, there should be no more than 1 probe active. The chat adapter will signal the release of probe before requesting for a new one. + * Sets the [`NetworkInformation` API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). + * When the `NetworkInformation` detect network changes or offline, it will disconnect the Web Socket and reconnect it. */ - networkProbe?: - | NetworkInformation - | { - minimumInterval?: number; - url: string | URL; - } - | ((init: { signal: AbortSignal }) => AbortSignal); + networkInformation?: NetworkInformation; } class StreamHandler implements BFSE.RequestHandler { @@ -148,32 +113,22 @@ export class DirectLineStreaming implements IBotConnection { private _botAgent = ''; - #networkProbe: ((init: { signal: AbortSignal }) => AbortSignal) | undefined; + #networkInformation: NetworkInformation | undefined; constructor(options: DirectLineStreamingOptions) { - // Rectifies options.probe. - const networkProbe = options?.networkProbe; - - if (typeof networkProbe === 'function') { - this.#networkProbe = networkProbe; - } else if (typeof networkProbe === 'undefined') { - // Intentionally left blank. - } else if ( - networkProbe instanceof EventTarget || - // We also accept `EventTargetLike`. - (typeof networkProbe['addEventListener'] === 'function' && typeof networkProbe['removeEventListener'] === 'function') - ) { - this.#networkProbe = ({ signal }: { signal: AbortSignal }) => - probeNetworkInformation(networkProbe as NetworkInformation, { signal }); - } else if ( - (typeof networkProbe.minimumInterval === 'number' || typeof networkProbe.minimumInterval === 'undefined') && - (typeof networkProbe.url === 'string' || networkProbe.url instanceof URL) + // Rectifies `options.networkInformation`. + const networkInformation = options?.networkInformation; + + if ( + typeof networkInformation === 'undefined' || + (typeof networkInformation.addEventListener === 'function' && + typeof networkInformation.removeEventListener === 'function' && + typeof networkInformation.type === 'string') ) { - this.#networkProbe = ({ signal }: { signal: AbortSignal }) => - probeREST(networkProbe.url, { minimumInterval: networkProbe.minimumInterval, signal }); + this.#networkInformation = networkInformation; } else { - throw new Error( - 'botframework-directlinejs: "networkProbe" option must be either a function returning an AbortSignal, an object, or undefined.' + console.warn( + 'botframework-directlinejs: "networkInformation" option specified must be a `NetworkInformation`-like instance.' ); } @@ -399,12 +354,9 @@ export class DirectLineStreaming implements IBotConnection { // This promise will resolve when it is disconnected. return new Promise(async (resolve, reject) => { try { - const probe: AbortSignal | undefined = - typeof this.#networkProbe === 'function' ? this.#networkProbe({ signal: abortController.signal }) : undefined; - - this.streamConnection = new WebSocketClientWithProbe({ + this.streamConnection = new WebSocketClientWithNetworkInformation({ disconnectionHandler: resolve, - probe, + networkInformation: this.#networkInformation, requestHandler: { processRequest: streamingRequest => { // If `streamConnection` is still current, allow call to `processRequest()`, otherwise, ignore calls to `processRequest()`. @@ -416,7 +368,7 @@ export class DirectLineStreaming implements IBotConnection { return this.theStreamHandler.processRequest(streamingRequest); } }, - url: wsUrl, + url: wsUrl }); this.queueActivities = true; diff --git a/src/streaming/NetworkInformationObserver.spec.ts b/src/streaming/NetworkInformationObserver.spec.ts index 257a5fff..4841c9a3 100644 --- a/src/streaming/NetworkInformationObserver.spec.ts +++ b/src/streaming/NetworkInformationObserver.spec.ts @@ -352,13 +352,13 @@ describe('A NetworkInformationObserver', () => { test('should not reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(2))); }); - describe.only('after 10 seconds', () => { + describe('after 10 seconds', () => { beforeEach(() => jest.advanceTimersByTimeAsync(10_000)); test('should have connected 3 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(3))); }); - describe.only('after 20 seconds', () => { + describe('after 20 seconds', () => { beforeEach(() => jest.advanceTimersByTimeAsync(20_000)); test('should have connected 4 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(4))); diff --git a/src/streaming/WebSocketClientWithNetworkInformation.test.ts b/src/streaming/WebSocketClientWithNetworkInformation.test.ts index 59914b8b..1ddbc3d6 100644 --- a/src/streaming/WebSocketClientWithNetworkInformation.test.ts +++ b/src/streaming/WebSocketClientWithNetworkInformation.test.ts @@ -13,27 +13,22 @@ jest.mock('../../node_modules/botframework-streaming/lib/webSocket/nodeWebSocket } })); +type NetworkInformationType = 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; + class MockNetworkInformation extends EventTarget { constructor() { super(); } - #type: 'none' | 'unknown' = 'none'; + #type: NetworkInformationType = 'none'; get type() { return this.#type; } - setOffline() { - if (this.#type !== 'none') { - this.#type = 'none'; - this.dispatchEvent(new Event('change')); - } - } - - setOnline() { - if (this.#type === 'none') { - this.#type = 'unknown'; + set type(value: NetworkInformationType) { + if (this.#type !== value) { + this.#type = value; this.dispatchEvent(new Event('change')); } } @@ -57,7 +52,7 @@ beforeEach(() => { client = new WebSocketClientWithNetworkInformation({ disconnectionHandler, - networkInformationConnection, + networkInformation: networkInformationConnection, requestHandler, url }); @@ -71,7 +66,9 @@ afterEach(() => jest.restoreAllMocks()); test('should not call disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(0)); describe('initially online', () => { - beforeEach(() => networkInformationConnection.setOnline()); + beforeEach(() => { + networkInformationConnection.type = 'wifi'; + }); describe('when connect() is called', () => { beforeEach(() => client.connect()); @@ -83,7 +80,9 @@ describe('initially online', () => { test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); describe('when offline', () => { - beforeEach(() => networkInformationConnection.setOffline()); + beforeEach(() => { + networkInformationConnection.type = 'none'; + }); // After disconnected() is called, there should be no extra calls for offline. test('should have no extra calls to disconnectHandler()', () => @@ -92,7 +91,9 @@ describe('initially online', () => { }); describe('when offline', () => { - beforeEach(() => networkInformationConnection.setOffline()); + beforeEach(() => { + networkInformationConnection.type = 'none'; + }); // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); diff --git a/src/streaming/WebSocketClientWithNetworkInformation.ts b/src/streaming/WebSocketClientWithNetworkInformation.ts index aed36709..1bd0bf21 100644 --- a/src/streaming/WebSocketClientWithNetworkInformation.ts +++ b/src/streaming/WebSocketClientWithNetworkInformation.ts @@ -9,7 +9,7 @@ type WebSocketClientWithNetworkInformationInit = { * Note: This function could be called multiple times, the callee is expected to ignore subsequent calls. */ disconnectionHandler: (message: string) => void; - networkInformationConnection: NetworkInformation; + networkInformation?: NetworkInformation | undefined; requestHandler: RequestHandler; url: string; }; @@ -17,7 +17,7 @@ type WebSocketClientWithNetworkInformationInit = { export default class WebSocketClientWithNetworkInformation extends WebSocketClient { constructor({ disconnectionHandler, - networkInformationConnection, + networkInformation, requestHandler, url }: WebSocketClientWithNetworkInformationInit) { @@ -27,11 +27,16 @@ export default class WebSocketClientWithNetworkInformation extends WebSocketClie url }); - this.#networkInformationConnection = networkInformationConnection; + this.#networkInformation = networkInformation; } #connectCalled: boolean = false; - #networkInformationConnection: NetworkInformation; + // According to W3C Network Information API, https://wicg.github.io/netinfo/#handling-changes-to-the-underlying-connection. + // NetworkInformation.onChange event will be fired on any changes to: `downlinkMax`, `type`, `downlink`, or `rtt`. + #handleNetworkInformationChange = () => + this.#initialNetworkInformationType === this.#networkInformation.type || this.disconnect(); + #initialNetworkInformationType: NetworkInformation['type']; + #networkInformation: NetworkInformation; // TODO: Better, the `NetworkInformation` instance should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. // `BrowserWebSocket` is where it creates `WebSocket` object. @@ -46,15 +51,25 @@ export default class WebSocketClientWithNetworkInformation extends WebSocketClie this.#connectCalled = true; - if (this.#networkInformationConnection.type === 'none') { - return Promise.reject(new Error('botframework-directlinejs: Failed to connect while offline.')); - } + if (this.#networkInformation) { + const { type: initialType } = this.#networkInformation; + + this.#initialNetworkInformationType = initialType; - this.#networkInformationConnection.addEventListener( - 'change', - () => this.#networkInformationConnection.type === 'none' && this.disconnect() - ); + if (initialType === 'none') { + console.warn('botframework-directlinejs: Failed to connect while offline.'); + + return Promise.reject(new Error('botframework-directlinejs: Failed to connect while offline.')); + } + + this.#networkInformation.addEventListener('change', this.#handleNetworkInformationChange); + } return super.connect(); } + + disconnect() { + this.#networkInformation.removeEventListener('change', this.#handleNetworkInformationChange); + super.disconnect(); + } } diff --git a/src/streaming/WebSocketClientWithProbe.integration.test.ts b/src/streaming/WebSocketClientWithProbe.integration.test.ts deleted file mode 100644 index 9cf892cd..00000000 --- a/src/streaming/WebSocketClientWithProbe.integration.test.ts +++ /dev/null @@ -1,77 +0,0 @@ -import type { WebSocketClient } from 'botframework-streaming'; -import type OriginalWebSocketClientWithProbe from './WebSocketClientWithProbe'; - -// Mocked modules are available across the test file. They cannot be unmocked. -// Thus, they are more-or-less similar to import/require. -jest.mock('../../node_modules/botframework-streaming/lib/webSocket/nodeWebSocket', () => ({ - __esmodule: true, - NodeWebSocket: class { - connect() {} - setOnCloseHandler() {} - setOnErrorHandler() {} - setOnMessageHandler() {} - } -})); - -type WebSocketClientInit = ConstructorParameters[0]; - -const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); -const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; -const url: string = 'wss://dummy/'; - -let client: WebSocketClient; -let probe: AbortController; - -beforeEach(() => { - probe = new AbortController(); - - let WebSocketClientWithProbe: typeof OriginalWebSocketClientWithProbe; - - WebSocketClientWithProbe = require('./WebSocketClientWithProbe').default; - - client = new WebSocketClientWithProbe({ - disconnectionHandler, - probe: probe.signal, - requestHandler, - url - }); - - // Spy on all `console.warn()`. - jest.spyOn(console, 'warn').mockImplementation(() => {}); -}); - -afterEach(() => jest.restoreAllMocks()); - -test('should not call disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(0)); - -describe('call connect()', () => { - beforeEach(() => client.connect()); - - describe('call disconnect()', () => { - beforeEach(() => client.disconnect()); - - // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. - test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); - - describe('followed by aborting the probe', () => { - beforeEach(() => probe.abort()); - - // After disconnected() is called, there should be no extra calls for aborting the signal. - test('should have no extra calls to disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(2)); - }); - }); - - describe('abort the probe', () => { - beforeEach(() => probe.abort()); - - // Both sender/receiver will call `onConnectionDisconnected`, so it is calling it twice. - test('should call disconnectHandler() twice', () => expect(disconnectionHandler).toBeCalledTimes(2)); - - describe('then call disconnect()', () => { - beforeEach(() => client.disconnect()); - - // After the signal is aborted, there should be no extra calls for calling disconnect(). - test('should have no extra calls to disconnectHandler()', () => expect(disconnectionHandler).toBeCalledTimes(2)); - }); - }); -}); diff --git a/src/streaming/WebSocketClientWithProbe.test.ts b/src/streaming/WebSocketClientWithProbe.test.ts deleted file mode 100644 index 0e6dd1ee..00000000 --- a/src/streaming/WebSocketClientWithProbe.test.ts +++ /dev/null @@ -1,138 +0,0 @@ -import type { WebSocketClient } from 'botframework-streaming'; -import type ActualWebSocketClientWithProbe from './WebSocketClientWithProbe'; - -// Mocked modules are available across the test file. They cannot be unmocked. -// Thus, they are more-or-less similar to import/require. -jest.mock('botframework-streaming', () => ({ - __esmodule: true, - WebSocketClient: class WebSocketClient { - constructor({ disconnectionHandler, requestHandler, url }: WebSocketClientInit) { - this.#disconnectionHandler = disconnectionHandler; - this.#requestHandler = requestHandler; - this.#url = url; - - // Set up mocks. - this.#connect = jest.fn(() => Promise.resolve()); - this.#disconnect = jest.fn(() => this.#disconnectionHandler?.('disconnect() is called')); - } - - #connect: () => Promise; - #disconnect: () => void; - #disconnectionHandler: WebSocketClientInit['disconnectionHandler']; - #requestHandler: WebSocketClientInit['requestHandler']; - #url: string; - - connect(): Promise { - return this.#connect(); - } - - disconnect(): void { - return this.#disconnect(); - } - - get __test__connect(): () => Promise { - return this.#connect; - } - - get __test__disconnect(): () => void { - return this.#disconnect; - } - - get __test__disconnectionHandler(): WebSocketClientInit['disconnectionHandler'] { - return this.#disconnectionHandler; - } - - get __test__requestHandler(): WebSocketClientInit['requestHandler'] { - return this.#requestHandler; - } - - get __test__url(): WebSocketClientInit['url'] { - return this.#url; - } - } -})); - -type WebSocketClientInit = ConstructorParameters[0]; - -const disconnectionHandler: WebSocketClientInit['disconnectionHandler'] = jest.fn(); -const requestHandler: WebSocketClientInit['requestHandler'] = { processRequest: jest.fn() }; -const url: string = 'wss://dummy/'; - -let client: WebSocketClient; -let probe: AbortController; - -beforeEach(() => { - probe = new AbortController(); - - let WebSocketClientWithProbe: typeof ActualWebSocketClientWithProbe; - - WebSocketClientWithProbe = require('./WebSocketClientWithProbe').default; - - client = new WebSocketClientWithProbe({ - disconnectionHandler, - probe: probe.signal, - requestHandler, - url - }); - - // Spy on all `console.warn()`. - jest.spyOn(console, 'warn').mockImplementation(() => {}); -}); - -afterEach(() => jest.restoreAllMocks()); - -describe('constructor', () => { - test('should pass `disconnectionHandler`', () => - expect(client['__test__disconnectionHandler']).toBe(disconnectionHandler)); - test('should pass `requestHandler`', () => expect(client['__test__requestHandler']).toBe(requestHandler)); - test('should pass `url`', () => expect(client['__test__url']).toBe(url)); -}); - -describe('connect()', () => { - test('should not call super.connect() initially', () => expect(client['__test__connect']).toBeCalledTimes(0)); - - describe('when called', () => { - beforeEach(() => client.connect()); - - test('should call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(1)); - - describe('twice', () => { - beforeEach(() => client.connect()); - - test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); - test('should warn "connect() can only be called once"', () => - expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('connect() can only be called once'))); - test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); - }); - - describe('then abort the probe', () => { - beforeEach(() => probe.abort()); - - test('should call disconnect()', () => expect(client['__test__disconnect']).toBeCalledTimes(1)); - - describe('when connect() is called again', () => { - beforeEach(() => client.connect()); - - test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); - test('should warn "connect() can only be called once"', () => - expect(console.warn).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('connect() can only be called once') - )); - test('should call super.connect() once', () => expect(client['__test__connect']).toBeCalledTimes(1)); - }); - }); - }); -}); - -describe('probe is aborted then call connect()', () => { - beforeEach(() => { - probe.abort(); - client.connect(); - }); - - test('should warn once', () => expect(console.warn).toBeCalledTimes(1)); - test('should warn "probe is aborted before connect()"', () => - expect(console.warn).toHaveBeenNthCalledWith(1, expect.stringContaining('probe is aborted before connect()'))); - test('should not call super.connect()', () => expect(client['__test__connect']).toBeCalledTimes(0)); -}); diff --git a/src/streaming/WebSocketClientWithProbe.ts b/src/streaming/WebSocketClientWithProbe.ts deleted file mode 100644 index f71ff6a9..00000000 --- a/src/streaming/WebSocketClientWithProbe.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { WebSocketClient } from 'botframework-streaming'; - -import type { RequestHandler } from 'botframework-streaming'; - -type WebSocketClientWithProbeInit = { - /** - * Gets or sets the observer function for disconnection or error sending/receiving through WebSocket. - * - * Note: This function could be called multiple times, the callee is expected to ignore subsequent calls. - */ - disconnectionHandler?: (message: string) => void; - probe?: AbortSignal; - requestHandler: RequestHandler; - url: string; -}; - -export default class WebSocketClientWithProbe extends WebSocketClient { - constructor({ disconnectionHandler, probe, requestHandler, url }: WebSocketClientWithProbeInit) { - super({ - disconnectionHandler, - requestHandler, - url - }); - - this.#probe = probe; - } - - #connectCalled: boolean = false; - #probe: WebSocketClientWithProbeInit['probe']; - - // TODO: Better, the `probe` should be passed to `BrowserWebSocketClient` -> `BrowserWebSocket`. - // `BrowserWebSocket` is where it creates `WebSocket` object. - // The `probe` object should accompany `WebSocket` and forcibly close it on abort. - // Maybe `botframework-streaming` should accept ponyfills. - connect(): Promise { - if (this.#connectCalled) { - console.warn('botframework-directlinejs: connect() can only be called once.'); - - return; - } else if (this.#probe?.aborted) { - console.warn('botframework-directlinejs: probe is aborted before connect().'); - - return; - } - - this.#connectCalled = true; - - // Note: It is expected disconnectionHandler() will be called multiple times. - // If we call disconnect(), both sender/receiver will call super.onConnectionDisconnected(), - // which in turn, call disconnectionHandler() twice, all from a single disconnect() call. - this.#probe?.addEventListener('abort', () => this.disconnect(), { once: true }); - - return super.connect(); - } -} diff --git a/src/streaming/probeNetworkInformation.test.ts b/src/streaming/probeNetworkInformation.test.ts deleted file mode 100644 index cd44a9e3..00000000 --- a/src/streaming/probeNetworkInformation.test.ts +++ /dev/null @@ -1,25 +0,0 @@ -import probeNetworkInformation from './probeNetworkInformation'; - -let abortController: AbortController; -let networkInformation: EventTarget; - -beforeEach(() => { - abortController = new AbortController(); - networkInformation = new EventTarget(); -}); - -describe('after constructed', () => { - let probe: AbortSignal; - - beforeEach(() => { - probe = probeNetworkInformation(networkInformation, { signal: abortController.signal }); - }); - - test('should not be aborted', () => expect(probe.aborted).toBe(false)); - - describe('when "change" event is received', () => { - beforeEach(() => networkInformation.dispatchEvent(new Event('change'))); - - test('should be aborted', () => expect(probe.aborted).toBe(true)); - }); -}); diff --git a/src/streaming/probeNetworkInformation.ts b/src/streaming/probeNetworkInformation.ts deleted file mode 100644 index 345c5af8..00000000 --- a/src/streaming/probeNetworkInformation.ts +++ /dev/null @@ -1,27 +0,0 @@ -/// - -type ProbeNetworkInformationInit = { - signal: AbortSignal; -}; - -/** - * Probes the connection a device is using to communicate with the network via [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API). - * - * When the connection change, the probe will treat it as a fault. - */ -export default function probeNetworkInformation( - connection: NetworkInformation, - { signal }: ProbeNetworkInformationInit -): AbortSignal { - const abortController = new AbortController(); - const handleNetworkInformationChange = () => abortController.abort(); - - connection.addEventListener('change', handleNetworkInformationChange, { once: true }); - - signal?.addEventListener('abort', () => { - connection.removeEventListener('change', handleNetworkInformationChange); - abortController.abort(); - }); - - return abortController.signal; -} diff --git a/src/streaming/probeREST.test.ts b/src/streaming/probeREST.test.ts deleted file mode 100644 index 95b7a57a..00000000 --- a/src/streaming/probeREST.test.ts +++ /dev/null @@ -1,230 +0,0 @@ -import { createServer } from 'http'; -import express from 'express'; -import hasResolved from 'has-resolved'; - -import createDeferred from '../createDeferred'; -import probeREST from './probeREST'; -import waitFor from './__setup__/waitFor'; - -import type { Deferred } from '../createDeferred'; -import type { IncomingMessage, Server, ServerResponse } from 'http'; -import type { Request, Response } from 'express'; - -const pollFn = jest.fn, [Request, Response], any>(async (_req, res) => { - const callIndex = pollFn.mock.calls.length - 1; - - pollCallDeferred[callIndex]?.resolve(); - - res.chunkedEncoding = true; - res.status(200); - res.setHeader('cache-control', 'no-transform'); - res.write(' '); - - const handleClose = () => pollReturnDeferred[callIndex]?.reject(new Error('Socket closed.')); - - res.once('close', handleClose); - - setTimeout(() => { - res.off('close', handleClose); - res.end(); - - pollReturnDeferred[callIndex]?.resolve(); - }, 30_000); -}); - -function setup(): Promise void]> { - return new Promise(resolve => { - const app = express(); - - app.get('/poll', pollFn); - - const server = createServer(app); - - server.listen(() => { - const url = new URL('http://localhost/poll'); - const address = server.address(); - - if (typeof address === 'string') { - url.host = address; - } else if (address) { - url.hostname = address.address; - url.port = address.port + ''; - } - - resolve(Object.freeze([url, server, () => server.closeAllConnections()])); - }); - }); -} - -let abortController: AbortController; -let pollCallDeferred: Deferred[]; -let pollReturnDeferred: Deferred[]; -let server: Server; -let serverURL: URL; -let signal: AbortSignal; -let teardownServer: () => void; - -beforeEach(async () => { - pollCallDeferred = new Array(5).fill(undefined).map(() => createDeferred()); - pollReturnDeferred = new Array(5).fill(undefined).map(() => { - const deferred = createDeferred(); - - deferred.promise.catch(() => {}); - - return deferred; - }); - - jest.clearAllMocks(); - jest.useFakeTimers({ advanceTimers: true, now: 0 }); - - [serverURL, server, teardownServer] = await setup(); - - abortController = new AbortController(); - - jest.spyOn(console, 'warn').mockImplementation(() => {}); -}); - -afterEach(() => { - teardownServer(); - - jest.useRealTimers(); - console.warn['mockRestore']?.(); - - return new Promise(resolve => { - signal.addEventListener('abort', () => resolve()); - - abortController.abort(); - - signal.aborted && resolve(); - }); -}); - -describe('with default options', () => { - beforeEach(() => { - signal = probeREST(serverURL, { signal: abortController.signal }); - }); - - describe('after first poll received', () => { - beforeEach(() => pollCallDeferred[0].promise); - - test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); - test('should not have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); - - describe('after 30 seconds', () => { - beforeEach(async () => { - jest.advanceTimersByTime(30_000); - - await pollCallDeferred[1].promise; - }); - - test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); - test('should have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); - test('should not warn', () => expect(console.warn).not.toBeCalled()); - - describe('after 30 seconds again', () => { - beforeEach(async () => { - jest.advanceTimersByTimeAsync(30_000); - - await pollCallDeferred[2].promise; - }); - - test('should have called API three times', () => waitFor(() => expect(pollFn).toBeCalledTimes(3))); - test('should have second API response', () => - expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(true)); - test('should not warn', () => expect(console.warn).not.toBeCalled()); - }); - }); - - describe('teardown server', () => { - beforeEach(() => teardownServer()); - - test('should abort the signal', () => waitFor(() => expect(signal.aborted).toBe(true))); - }); - - describe('when aborted', () => { - beforeEach(() => abortController.abort()); - - test('should signal', () => expect(signal.aborted).toBe(true)); - test('server should get the "close" event', () => - expect(() => pollReturnDeferred[0].promise).rejects.toThrow('Socket closed.')); - }); - }); -}); - -describe('with minimumInterval=45_000', () => { - beforeEach(() => { - signal = probeREST(serverURL, { - minimumInterval: 45_000, - signal: abortController.signal - }); - }); - - describe('after first poll received', () => { - beforeEach(() => pollCallDeferred[0].promise); - - test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); - test('should not have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); - - describe('after 30 seconds', () => { - beforeEach(() => jest.advanceTimersByTime(30_000)); - - test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); - test('should have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); - test('should warn once', () => - waitFor(() => - expect(console.warn).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('REST API should not return sooner than the predefined `minimumInterval` of 45000 ms.') - ) - )); - - describe('after 30 seconds', () => { - beforeEach(() => jest.advanceTimersByTime(30_000)); - - test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); - test('should not have second API response', () => - expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(false)); - }); - }); - }); -}); - -describe('with minimumInterval=15_000', () => { - beforeEach(() => { - signal = probeREST(serverURL, { - minimumInterval: 15_000, - signal: abortController.signal - }); - }); - - describe('after first poll received', () => { - beforeEach(() => pollCallDeferred[0].promise); - - test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); - test('should not have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); - - describe('after 20 seconds', () => { - beforeEach(() => jest.advanceTimersByTime(20_000)); - - test('should have called API once', () => expect(pollFn).toBeCalledTimes(1)); - test('should not have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(false)); - - describe('after 10 seconds', () => { - beforeEach(() => jest.advanceTimersByTime(10_000)); - - test('should have called API twice', () => waitFor(() => expect(pollFn).toBeCalledTimes(2))); - test('should have first API response', () => - expect(hasResolved(pollReturnDeferred[0].promise)).resolves.toBe(true)); - test('should not have second API response', () => - expect(hasResolved(pollReturnDeferred[1].promise)).resolves.toBe(false)); - test('should not warn', () => expect(console.warn).not.toBeCalled()); - }); - }); - }); -}); diff --git a/src/streaming/probeREST.ts b/src/streaming/probeREST.ts deleted file mode 100644 index 4fdef6e1..00000000 --- a/src/streaming/probeREST.ts +++ /dev/null @@ -1,98 +0,0 @@ -import sleep from './sleep'; - -// The liveness service should respond HTTP 2xx not sooner than 30 seconds. -// To prevent DoS the liveness service, we should not send probe to the liveness service sooner than 25 seconds. -let DEFAULT_MINIMUM_INTERVAL = 25_000; -let MINIMUM_MINIMUM_INTERVAL = 10_000; - -type ProbeRESTInit = { - /** - * Minimum time between pings in milliseconds, minimum is 10 seconds, default to 25 seconds. - * - * The probing REST API endpoint should keep the polling call open for at least 30 seconds, then respond with HTTP 2xx. - * - * If the service respond with HTTP 2xx sooner than this interval, the next call will be delayed until the interval has passed. - * In the meantime, the probe will not be able to detect any connection faults. - * - * If the service respond with HTTP 2xx on or later than this interval, the next call will be made immediately. - * - * For example, assumes the interval is set to 25 seconds. If the service responded after 10 seconds the call is being made, - * the probe will wait for 15 seconds before making another call. - */ - minimumInterval?: number; - - /** - * Signal to abort the probe. - * - * When this signal is being aborted, the probe will treat it as a fault. - */ - signal?: AbortSignal; -}; - -/** - * Probes connectivity issues by continuously calling a long-polling REST API endpoint and returns an `AbortSignal` object. - * - * The returned `AbortSignal` object will be aborted when: - * - * 1. the REST API endpoint did not return a HTTP 2xx status, or; - * 2. the `ProbeRESTInit.signal` is aborted. - * - * The REST API endpoint should keep the polling call open for 30 seconds, then respond with HTTP 2xx. - * - * Upon receiving HTTP 2xx, the probe will issue another long polling call immediately and not sooner than `minimumInterval`. - * - * [RFC6202](https://www.rfc-editor.org/rfc/rfc6202) recommends using a timeout value of 30 seconds. - */ -export default function probeREST(url: string | URL, { minimumInterval, signal }: ProbeRESTInit = {}): AbortSignal { - const abortController = new AbortController(); - - (async () => { - let warnedReturnTooSoon = false; - - try { - // Rectifying `minimumInterval`. - if (typeof minimumInterval === 'number') { - minimumInterval = Math.max(MINIMUM_MINIMUM_INTERVAL, minimumInterval); - } else { - minimumInterval = DEFAULT_MINIMUM_INTERVAL; - } - - // Rectifying `signal`. - if (signal && !(signal instanceof AbortSignal)) { - signal = undefined; - } - - // Instead of listening to signal.addEventListener('abort'), we will just let fetch() handle all aborts. - while (!signal?.aborted) { - const pingAt = Date.now(); - const res = await fetch(url, { signal }); - - if (!res.ok) { - throw new Error('Failed to ping REST endpoint.'); - } - - await res.arrayBuffer(); - - // TODO: Warns if the API returns sooner than `minimumInterval`. - const timeToSleep = pingAt + minimumInterval - Date.now(); - - if (timeToSleep > 0) { - warnedReturnTooSoon || - console.warn( - `botframework-directlinejs: REST API should not return sooner than the predefined \`minimumInterval\` of ${minimumInterval} ms.` - ); - - warnedReturnTooSoon = true; - } - - await sleep(timeToSleep, { signal }); - } - } catch (error) { - // If any error occurred, probably it is about connection issues. - } finally { - abortController.abort(); - } - })(); - - return abortController.signal; -} From 8cad8489799d921444dce634b2bc6fd27c731ccb Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:21:16 -0700 Subject: [PATCH 20/33] Fix flaky --- .../directLineStreaming/connect.fail.story.js | 2 +- .../postActivity.fail.story.js | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/__tests__/directLineStreaming/connect.fail.story.js b/__tests__/directLineStreaming/connect.fail.story.js index 23adfada..ddb9fb94 100644 --- a/__tests__/directLineStreaming/connect.fail.story.js +++ b/__tests__/directLineStreaming/connect.fail.story.js @@ -43,7 +43,7 @@ test('connect fail should signal properly', async () => { directLine.activity$.subscribe(activityObserver); // THEN: Should try to connect 3 times. - await waitFor(() => expect(onUpgrade).toBeCalledTimes(3)); + await waitFor(() => expect(onUpgrade).toBeCalledTimes(3), { timeout: 5_000 }); // THEN: Should not wait before connecting the first time. expect(onUpgrade.mock.results[0].value - connectTime).toBeLessThan(3000); diff --git a/__tests__/directLineStreaming/postActivity.fail.story.js b/__tests__/directLineStreaming/postActivity.fail.story.js index 9e786187..fb656e0c 100644 --- a/__tests__/directLineStreaming/postActivity.fail.story.js +++ b/__tests__/directLineStreaming/postActivity.fail.story.js @@ -79,13 +79,15 @@ test('should send activity', async () => { ); // THEN: Should observe "Connecting" -> "Online" because the chat adapter should reconnect. - await waitFor(() => - expect(connectionStatusObserver).toHaveProperty('observations', [ - [expect.any(Number), 'next', ConnectionStatus.Uninitialized], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online], - [expect.any(Number), 'next', ConnectionStatus.Connecting], - [expect.any(Number), 'next', ConnectionStatus.Online], - ]) + await waitFor( + () => + expect(connectionStatusObserver).toHaveProperty('observations', [ + [expect.any(Number), 'next', ConnectionStatus.Uninitialized], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online], + [expect.any(Number), 'next', ConnectionStatus.Connecting], + [expect.any(Number), 'next', ConnectionStatus.Online] + ]), + { timeout: 5_000 } ); }, 15000); From 7fab0f1d9899c9cb8e0dd401c113aac26a234a1a Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:21:33 -0700 Subject: [PATCH 21/33] Clean up --- .../__setup__/createBotProxy.ts | 44 +------------------ .../__setup__/setupBotProxy.ts | 12 +---- 2 files changed, 2 insertions(+), 54 deletions(-) diff --git a/__tests__/directLineStreaming/__setup__/createBotProxy.ts b/__tests__/directLineStreaming/__setup__/createBotProxy.ts index beec3305..dd0322c1 100644 --- a/__tests__/directLineStreaming/__setup__/createBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/createBotProxy.ts @@ -29,14 +29,9 @@ type CreateBotProxyInit = { type CreateBotProxyReturnValue = { cleanUp: () => void; - closeAllNetworkProbingConnections: () => void; closeAllWebSocketConnections: () => void; directLineStreamingURL: string; directLineURL: string; - networkProbeURL: string; - - get numNetworkProbingConnection(): number; - get numOverTheLifetimeNetworkProbingConnection(): number; }; const matchDirectLineStreamingProtocol = match('/.bot/', { decode: decodeURIComponent, end: false }); @@ -102,30 +97,6 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { - const destroyResponse = () => { - res.destroy(); - }; - - closeAllNetworkProbingConnections.push(destroyResponse); - numOverTheLifetimeNetworkProbingConnection++; - - req.once('error', destroyResponse); - res.once('close', () => removeInline(closeAllNetworkProbingConnections, destroyResponse)); - - res.chunkedEncoding = true; - res.statusCode = 200; - - res.setHeader('cache-control', 'no-cache'); - res.setHeader('content-type', 'text/plain'); - res.write(' '); - - setTimeout(() => { - req.off('error', destroyResponse); - res.end(); - }, 30_000); - }); - const webSocketProxy = new WebSocketServer({ noServer: true }); webSocketProxy.on('connection', (socket: WebSocket, proxySocket: WebSocket, req: IncomingMessage) => { @@ -206,22 +177,9 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise { - closeAllNetworkProbingConnections.forEach(close => close()); - closeAllNetworkProbingConnections.splice(0); - }, closeAllWebSocketConnections, directLineStreamingURL: new URL('/.bot/v3/directline', url).href, - directLineURL: new URL('/v3/directline', url).href, - networkProbeURL: new URL('/test/network-probe', url).href, - - get numNetworkProbingConnection(): number { - return closeAllNetworkProbingConnections.length; - }, - - get numOverTheLifetimeNetworkProbingConnection(): number { - return numOverTheLifetimeNetworkProbingConnection; - } + directLineURL: new URL('/v3/directline', url).href }); }); } catch (error) { diff --git a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts index 54156ae1..aa7ccfee 100644 --- a/__tests__/directLineStreaming/__setup__/setupBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/setupBotProxy.ts @@ -19,19 +19,9 @@ export default async function setupBotProxy( botProxies.push(botProxy); return { - closeAllNetworkProbingConnections: botProxy.closeAllNetworkProbingConnections, closeAllWebSocketConnections: botProxy.closeAllWebSocketConnections, directLineURL: botProxy.directLineURL, - directLineStreamingURL: botProxy.directLineStreamingURL, - networkProbeURL: botProxy.networkProbeURL, - - get numNetworkProbingConnection() { - return botProxy.numNetworkProbingConnection; - }, - - get numOverTheLifetimeNetworkProbingConnection() { - return botProxy.numOverTheLifetimeNetworkProbingConnection; - } + directLineStreamingURL: botProxy.directLineStreamingURL }; } From e5b0eb1b24d381248f0cce208163c4544ea2ca1c Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:21:44 -0700 Subject: [PATCH 22/33] Explain NetworkInformation requirement --- src/directLineStreaming.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/directLineStreaming.ts b/src/directLineStreaming.ts index da107cd6..fabc3114 100644 --- a/src/directLineStreaming.ts +++ b/src/directLineStreaming.ts @@ -128,7 +128,7 @@ export class DirectLineStreaming implements IBotConnection { this.#networkInformation = networkInformation; } else { console.warn( - 'botframework-directlinejs: "networkInformation" option specified must be a `NetworkInformation`-like instance.' + 'botframework-directlinejs: "networkInformation" option specified must be a `NetworkInformation`-like instance extending `EventTarget` interface with a `type` property returning a string.' ); } From 39497c2af1cc23bc0ed588fe39787f11ea9e2934 Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:22:04 -0700 Subject: [PATCH 23/33] Allow unset networkInformation option --- src/streaming/WebSocketClientWithNetworkInformation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/streaming/WebSocketClientWithNetworkInformation.ts b/src/streaming/WebSocketClientWithNetworkInformation.ts index 1bd0bf21..b3ccdd0e 100644 --- a/src/streaming/WebSocketClientWithNetworkInformation.ts +++ b/src/streaming/WebSocketClientWithNetworkInformation.ts @@ -69,7 +69,7 @@ export default class WebSocketClientWithNetworkInformation extends WebSocketClie } disconnect() { - this.#networkInformation.removeEventListener('change', this.#handleNetworkInformationChange); + this.#networkInformation?.removeEventListener('change', this.#handleNetworkInformationChange); super.disconnect(); } } From c99fa22732b2b36ce15083f6eae7644023923d82 Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 16:26:46 -0700 Subject: [PATCH 24/33] Clean up --- CHANGELOG.md | 2 +- .../directLineStreaming/options.networkInformation.story.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 249fe895..39ad7588 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added -- Direct Line Streaming: Added `networkProbe` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) +- Direct Line Streaming: Added `networkInformation` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) ## [0.15.4] - 2023-06-05 diff --git a/__tests__/directLineStreaming/options.networkInformation.story.ts b/__tests__/directLineStreaming/options.networkInformation.story.ts index 2200d5f0..3e16202a 100644 --- a/__tests__/directLineStreaming/options.networkInformation.story.ts +++ b/__tests__/directLineStreaming/options.networkInformation.story.ts @@ -16,8 +16,8 @@ const TOKEN_URL = 'https://webchat-mockbot3.azurewebsites.net/api/token/directli jest.setTimeout(10_000); -// GIVEN: A Direct Line Streaming chat adapter with network probe on Network Information API. -describe('Direct Line Streaming chat adapter with network probe on Network Information API', () => { +// GIVEN: A Direct Line Streaming chat adapter with Network Information API. +describe('Direct Line Streaming chat adapter with Network Information API', () => { let activityObserver: MockObserver; let botProxy: ResultOfPromise>; let connectionStatusObserver: MockObserver; From 8aacde8d6a53b040eac70803356c3c377da44fd3 Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 10 Jul 2023 18:15:00 -0700 Subject: [PATCH 25/33] Clean up --- __tests__/directLineStreaming/__setup__/createBotProxy.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/__tests__/directLineStreaming/__setup__/createBotProxy.ts b/__tests__/directLineStreaming/__setup__/createBotProxy.ts index dd0322c1..204dd78a 100644 --- a/__tests__/directLineStreaming/__setup__/createBotProxy.ts +++ b/__tests__/directLineStreaming/__setup__/createBotProxy.ts @@ -48,8 +48,6 @@ export default function createBotProxy(init?: CreateBotProxyInit): Promise void)[] = []; - let numOverTheLifetimeNetworkProbingConnection = 0; streamingBotURL && app.use('/.bot/', createProxyMiddleware({ changeOrigin: true, logLevel: 'silent', target: streamingBotURL })); From 2b2c85b6586008e8f1c51e12152a995a91eb6a72 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 01:14:06 -0700 Subject: [PATCH 26/33] Remove NetworkInformation polyfill --- src/streaming/NetworkInformation.d.ts | 24 -- .../NetworkInformationObserver.spec.ts | 367 ------------------ src/streaming/NetworkInformationObserver.ts | 154 -------- src/streaming/__setup__/waitFor.ts | 71 ---- src/streaming/mergeAbortSignal.spec.ts | 36 -- src/streaming/mergeAbortSignal.ts | 19 - src/streaming/sleep.spec.ts | 73 ---- src/streaming/sleep.ts | 27 -- 8 files changed, 771 deletions(-) delete mode 100644 src/streaming/NetworkInformation.d.ts delete mode 100644 src/streaming/NetworkInformationObserver.spec.ts delete mode 100644 src/streaming/NetworkInformationObserver.ts delete mode 100644 src/streaming/__setup__/waitFor.ts delete mode 100644 src/streaming/mergeAbortSignal.spec.ts delete mode 100644 src/streaming/mergeAbortSignal.ts delete mode 100644 src/streaming/sleep.spec.ts delete mode 100644 src/streaming/sleep.ts diff --git a/src/streaming/NetworkInformation.d.ts b/src/streaming/NetworkInformation.d.ts deleted file mode 100644 index 12c6e96f..00000000 --- a/src/streaming/NetworkInformation.d.ts +++ /dev/null @@ -1,24 +0,0 @@ -declare global { - // This is subset of https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation. - interface NetworkInformation extends EventTarget { - addEventListener( - type: 'change', - listener: EventListener | EventListenerObject, - options?: AddEventListenerOptions | boolean - ): void; - - removeEventListener( - type: 'change', - listener: EventListener | EventListenerObject, - options?: AddEventListenerOptions | boolean - ): void; - - get type(): 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; - } - - interface Navigator { - get connection(): NetworkInformation; - } -} - -export {} diff --git a/src/streaming/NetworkInformationObserver.spec.ts b/src/streaming/NetworkInformationObserver.spec.ts deleted file mode 100644 index 4841c9a3..00000000 --- a/src/streaming/NetworkInformationObserver.spec.ts +++ /dev/null @@ -1,367 +0,0 @@ -/// - -import { createServer, IncomingMessage } from 'http'; -import express from 'express'; - -import NetworkInformationObserver from './NetworkInformationObserver'; -import waitFor from './__setup__/waitFor'; - -import type { Response } from 'express'; -import type { Socket } from 'net'; - -beforeEach(() => jest.useFakeTimers({ advanceTimers: true, now: 0 })); -afterEach(() => jest.useRealTimers()); - -let baseURL: URL; -let firstChunkInterval: number; -let processRequest: jest.Mock; -let server: ReturnType; -let sockets: Set; - -beforeEach(async () => { - const app = express(); - - firstChunkInterval = 0; - sockets = new Set(); - processRequest = jest.fn(); - - app.get('/api/poll', (req, res) => { - processRequest(req, res); - sockets.add(req.socket); - - req.once('close', () => { - res.destroy(); - sockets.delete(req.socket); - }); - - const timeout = +(new URL(req.url, 'http://localhost/').searchParams.get('timeout') || 0); - - res.statusCode = 200; - res.setHeader('cache-control', 'no-store'); - res.setHeader('transfer-encoding', 'chunked'); - - if (timeout) { - setTimeout(() => { - res.write(' '); - - setTimeout(() => res.end(), timeout * 1_000 - firstChunkInterval); - }, firstChunkInterval); - } else { - res.end(); - } - }); - - app.get('/api/shortpoll', (req, res) => { - processRequest(req, res); - - res.statusCode = 204; - res.setHeader('cache-control', 'no-store'); - res.end(); - }); - - app.get('/api/longpoll', (req, res) => { - processRequest(req, res); - sockets.add(req.socket); - - res.statusCode = 200; - res.setHeader('cache-control', 'no-store'); - res.setHeader('transfer-encoding', 'chunked'); - res.write(' '); - - req.once('close', () => { - res.destroy(); - sockets.delete(req.socket); - }); - - setTimeout(() => res.end(), 30_000); - }); - - server = createServer(app); - - await new Promise(resolve => server.listen(0, 'localhost', resolve)); - - const address = server.address(); - - if (!address) { - throw new Error('Cannot listen.'); - } - - baseURL = new URL(`http://${typeof address === 'string' ? address : `${address.address}:${address.port}`}`); -}); - -afterEach(() => { - server.closeAllConnections(); - server.close(); - - jest.resetAllMocks(); -}); - -describe('A NetworkInformationObserver', () => { - let callback: jest.Mock; - let observer: NetworkInformationObserver; - - beforeEach(() => { - observer = new NetworkInformationObserver((callback = jest.fn())); - }); - - describe.each<[string, string, string?]>([ - ['default options', '/api/poll'], - ['shortPollURL', '/api/longpoll', '/api/shortpoll'] - ])('observe with %s', (_, url, shortPollURL) => { - let expectedLongPollURL: URL; - let expectedShortPollURL: URL; - - beforeEach(() => { - if (shortPollURL) { - observer.observe(new URL(url, baseURL).href, { shortPollURL: new URL(shortPollURL, baseURL) }); - - expectedLongPollURL = new URL(url, baseURL); - expectedShortPollURL = new URL(shortPollURL, baseURL); - } else { - observer.observe(new URL(url, baseURL).href); - - expectedLongPollURL = new URL(url, baseURL); - expectedLongPollURL.searchParams.set('timeout', '30'); - expectedShortPollURL = new URL(url, baseURL); - expectedShortPollURL.searchParams.set('timeout', '0'); - } - }); - - afterEach(() => observer.disconnect()); - - describe('when callback is received', () => { - let connection: NetworkInformation; - let handleChange: jest.Mock; - - beforeEach(() => { - expect(callback).toBeCalledTimes(1); - connection = callback.mock.calls[0][0]; - connection.addEventListener('change', (handleChange = jest.fn())); - }); - - test('should receive an instance of NetworkInformation', () => { - expect(callback).toHaveBeenNthCalledWith( - 1, - expect.objectContaining({ - type: expect.stringMatching(/^(bluetooth|cellular|ethernet|none|other|unknown|wifi|wimax)$/) - }) - ); - expect(typeof callback.mock.calls[0][0].addEventListener).toBe('function'); - expect(typeof callback.mock.calls[0][0].removeEventListener).toBe('function'); - }); - - describe('when connection is detected', () => { - beforeEach(() => - waitFor(() => { - expect(connection).toHaveProperty('type', 'unknown'); - expect(processRequest).toHaveBeenCalledTimes(2); - }) - ); - - test('should have type of "unknown"', () => - waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); - test('should dispatch "change" event', () => waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); - test('should connect with short poll followed by long poll', () => - waitFor(() => { - expect(processRequest).toHaveBeenCalledTimes(2); - expect(processRequest).toHaveBeenNthCalledWith( - 1, - expect.objectContaining({ url: expectedShortPollURL.pathname + expectedShortPollURL.search }), - expect.anything() - ); - expect(processRequest).toHaveBeenNthCalledWith( - 2, - expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), - expect.anything() - ); - expect(sockets).toHaveProperty('size', 1); - })); - - describe('when connection is broken', () => { - beforeEach(() => { - for (const socket of sockets.values()) { - socket.destroy(); - } - }); - - test('should have type of "none"', () => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); - test('should dispatch "change" event', () => waitFor(() => expect(handleChange).toHaveBeenCalledTimes(2))); - - describe('after disconnection is detected', () => { - beforeEach(() => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); - - describe('after 1 second', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(1_000)); - - test('should not reconnect', () => expect(sockets).toHaveProperty('size', 0)); - }); - - describe('after 2 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(2_000)); - - test('should have type of "unknown"', () => - waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); - test('should dispatch "change" event', () => - waitFor(() => expect(handleChange).toHaveBeenCalledTimes(3))); - test('should call short-poll followed by long-poll', () => - waitFor(() => { - expect(processRequest).toHaveBeenCalledTimes(4); - expect(processRequest).toHaveBeenNthCalledWith( - 3, - expect.objectContaining({ url: expectedShortPollURL.pathname + expectedShortPollURL.search }), - expect.anything() - ); - expect(processRequest).toHaveBeenNthCalledWith( - 4, - expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), - expect.anything() - ); - expect(sockets).toHaveProperty('size', 1); - })); - }); - }); - }); - - describe('after 30 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(30_000)); - - test('should not dispatch additional "change" event', () => - waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); - test('should connect with long poll', () => - waitFor(() => { - expect(processRequest).toHaveBeenCalledTimes(3); - expect(processRequest).toHaveBeenNthCalledWith( - 3, - expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), - expect.anything() - ); - expect(sockets).toHaveProperty('size', 1); - })); - - describe('after another 30 seconds', () => { - beforeEach(async () => { - await waitFor(() => expect(processRequest).toHaveBeenCalledTimes(3)); - await jest.advanceTimersByTimeAsync(30_000); - }); - - test('should not dispatch additional "change" event', () => - waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1))); - test('should connect with long poll', () => - waitFor(() => { - expect(processRequest).toHaveBeenCalledTimes(4); - expect(processRequest).toHaveBeenNthCalledWith( - 4, - expect.objectContaining({ url: expectedLongPollURL.pathname + expectedLongPollURL.search }), - expect.anything() - ); - expect(sockets).toHaveProperty('size', 1); - })); - }); - }); - }); - }); - }); - - describe('observe with first chunk delayed for 10 seconds', () => { - let connection: NetworkInformation; - - beforeEach(async () => { - firstChunkInterval = 10_000; - observer.observe(new URL('/api/poll', baseURL).href); - - await expect(callback).toBeCalledTimes(1); - connection = callback.mock.calls[0][0]; - }); - - afterEach(() => observer.disconnect()); - - test('should connect long poll', () => waitFor(() => expect(processRequest).toBeCalledTimes(2))); - - describe('when online', () => { - beforeEach(() => waitFor(() => expect(processRequest).toBeCalledTimes(2))); - - test('type should become "unknown"', () => waitFor(() => expect(connection).toHaveProperty('type', 'unknown'))); - - describe('after 5 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(5_000)); - - test('type should become "none"', () => waitFor(() => expect(connection).toHaveProperty('type', 'none'))); - test('should have closed the connection', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); - }); - }); - }); - - describe('observe and online', () => { - let connection: NetworkInformation; - - beforeEach(async () => { - observer.observe(new URL('/api/poll', baseURL).href); - - await expect(callback).toBeCalledTimes(1); - connection = callback.mock.calls[0][0]; - - await waitFor(() => expect(processRequest).toBeCalledTimes(2)); - waitFor(() => expect(connection).toHaveProperty('type', 'unknown')); - waitFor(() => expect(sockets).toHaveProperty('size', 1)); - }); - - afterEach(() => observer.disconnect()); - - describe('when unobserve() is called', () => { - beforeEach(() => observer.unobserve(new URL('/api/poll', baseURL).href)); - - test('should disconnect the call', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); - }); - - describe('when disconnect() is called', () => { - beforeEach(() => observer.disconnect()); - - test('should disconnect the call', () => waitFor(() => expect(sockets).toHaveProperty('size', 0))); - }); - }); - - describe('observe with short polling API', () => { - let connection: NetworkInformation; - - beforeEach(async () => { - jest.spyOn(console, 'warn').mockImplementation(() => {}); - - observer.observe(new URL('/api/shortpoll', baseURL).href); - - await expect(callback).toBeCalledTimes(1); - connection = callback.mock.calls[0][0]; - - await waitFor(() => expect(processRequest).toBeCalledTimes(2)); - waitFor(() => expect(connection).toHaveProperty('type', 'unknown')); - waitFor(() => expect(sockets).toHaveProperty('size', 1)); - }); - - afterEach(() => console.warn['mockRestore']?.()); - - test('should warn', () => { - expect(console.warn).toBeCalledTimes(1); - expect(console.warn).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('should not return sooner than 10 seconds') - ); - }); - - describe('after 5 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(5_000)); - - test('should not reconnect', () => waitFor(() => expect(processRequest).toBeCalledTimes(2))); - }); - - describe('after 10 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(10_000)); - - test('should have connected 3 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(3))); - }); - - describe('after 20 seconds', () => { - beforeEach(() => jest.advanceTimersByTimeAsync(20_000)); - - test('should have connected 4 times', () => waitFor(() => expect(processRequest).toBeCalledTimes(4))); - }); - }); -}); diff --git a/src/streaming/NetworkInformationObserver.ts b/src/streaming/NetworkInformationObserver.ts deleted file mode 100644 index 9a5b1ea1..00000000 --- a/src/streaming/NetworkInformationObserver.ts +++ /dev/null @@ -1,154 +0,0 @@ -import mergeAbortSignal from './mergeAbortSignal'; -import sleep from './sleep'; - -type NetworkInformationObserverCallback = (networkInformation: NetworkInformation) => void; -// type NetworkInformationType = 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; -type ObserveInit = { firstChunkWithin?: number; shortPollURL?: undefined | URL }; - -const DEFAULT_FIRST_CHUNK_WITHIN = 5_000; -const MINIMUM_PING_INTERVAL = 10_000; -const SLEEP_INTERVAL_AFTER_ERROR = 2_000; -const TIMEOUT_PARAM_NAME = 'timeout'; - -class NetworkInformationPolyfill extends EventTarget implements NetworkInformation { - constructor(url: URL, init: Required & { signal: AbortSignal }) { - super(); - - this.#firstChunkWithin = init.firstChunkWithin; - this.#shortPollURL = init.shortPollURL; - this.#signal = init.signal; - this.#type = 'none'; - this.#url = url; - - this.#start().catch(() => {}); - } - - #firstChunkWithin: number; - #shortPollURL: URL; - #signal: AbortSignal; - #type: 'none' | 'unknown'; - #url: URL; - - get type() { - return this.#type; - } - - #goOffline() { - this.#setType('none'); - } - - #goOnline() { - this.#setType('unknown'); - } - - #setType(type: 'none' | 'unknown') { - if (this.#type !== type) { - this.#type = type; - this.dispatchEvent(new Event('change')); - } - } - - async #start() { - let shortPing = true; - - while (!this.#signal.aborted) { - const currentAbortController = new AbortController(); - - try { - const signal = mergeAbortSignal(currentAbortController.signal, this.#signal); - const startTime = Date.now(); - - const res = await Promise.race([ - fetch(shortPing ? this.#shortPollURL : this.#url, { - headers: { 'cache-control': 'no-store' }, - signal - }), - sleep(this.#firstChunkWithin, { signal }).then(() => - Promise.reject(new Error('Timed out while waiting for first chunk.')) - ) - ]); - - // Received first chunk. - this.#goOnline(); - await res.arrayBuffer(); - - const timeToSleep = Math.max(0, MINIMUM_PING_INTERVAL + startTime - Date.now()); - - // Do not ping long-polling sooner than 10 seconds. - if (!shortPing) { - if (timeToSleep > 0) { - console.warn( - `botframework-directlinejs: network connectivity probe should not return sooner than 10 seconds.` - ); - - await sleep(timeToSleep, { signal }); - } - } - - shortPing = false; - } catch (error) { - currentAbortController.abort(); - - this.#goOffline(); - shortPing = true; - - await sleep(SLEEP_INTERVAL_AFTER_ERROR, { signal: this.#signal }); - } - } - } -} - -export default class NetworkInformationObserver { - constructor(callback: NetworkInformationObserverCallback) { - this.#callback = callback; - } - - #callback: NetworkInformationObserverCallback; - #connections: Map = new Map(); - - disconnect() { - for (const [_, abortController] of this.#connections.values()) { - abortController.abort(); - } - - this.#connections.clear(); - } - - observe(url: string, init: ObserveInit = {}) { - let entry = this.#connections.get(url); - - if (!entry) { - const rectifiedFirstChunkWithin = init.firstChunkWithin || DEFAULT_FIRST_CHUNK_WITHIN; - let rectifiedShortPollURL: URL; - let rectifiedURL: URL; - - if (init.shortPollURL) { - rectifiedShortPollURL = new URL(init.shortPollURL); - rectifiedURL = new URL(url); - } else { - rectifiedShortPollURL = new URL(url); - rectifiedShortPollURL.searchParams.set(TIMEOUT_PARAM_NAME, '0'); - rectifiedURL = new URL(url); - rectifiedURL.searchParams.set(TIMEOUT_PARAM_NAME, '30'); - } - - const abortController = new AbortController(); - - const connection = new NetworkInformationPolyfill(rectifiedURL, { - firstChunkWithin: rectifiedFirstChunkWithin, - shortPollURL: rectifiedShortPollURL, - signal: abortController.signal - }); - - entry = Object.freeze([connection, abortController]); - this.#connections.set(url, entry); - } - - this.#callback(entry[0]); - } - - unobserve(url: string) { - this.#connections.get(url)?.[1].abort(); - this.#connections.delete(url); - } -} diff --git a/src/streaming/__setup__/waitFor.ts b/src/streaming/__setup__/waitFor.ts deleted file mode 100644 index 057f77e6..00000000 --- a/src/streaming/__setup__/waitFor.ts +++ /dev/null @@ -1,71 +0,0 @@ -const DEFAULT_DURATION = 1000; -const DEFAULT_INTERVAL = 50; - -function isPromise(value: T | Promise): value is Promise { - return value && typeof (value as Promise).then === 'function'; -} - -export default function waitFor( - fn: () => Promise | T, - { - duration, - interval - }: { - duration?: number; - interval?: number; - } = {} -): Promise { - let runInterval: ReturnType; - let runTimeout: ReturnType; - - const promise = new Promise((resolve, reject) => { - let lastError: any; - let ready = true; - - const run = () => { - if (!ready) { - return; - } - - ready = false; - - let returnValue; - - try { - returnValue = fn(); - } catch (error) { - lastError = error; - ready = true; - - return; - } - - if (isPromise(returnValue)) { - returnValue.then( - result => resolve(result), - error => { - lastError = error; - ready = true; - } - ); - } else { - resolve(returnValue); - } - }; - - runInterval = setInterval(run, interval || DEFAULT_INTERVAL); - - runTimeout = setTimeout(() => { - reject(lastError || new Error('timed out')); - }, duration || DEFAULT_DURATION); - - run(); - }).finally(() => { - clearInterval(runInterval); - clearTimeout(runTimeout); - }); - - promise.catch(() => {}); - - return promise; -} diff --git a/src/streaming/mergeAbortSignal.spec.ts b/src/streaming/mergeAbortSignal.spec.ts deleted file mode 100644 index 403e1aff..00000000 --- a/src/streaming/mergeAbortSignal.spec.ts +++ /dev/null @@ -1,36 +0,0 @@ -import mergeAbortSignal from './mergeAbortSignal'; - -describe('merging 3 abort signals', () => { - let abortControllers: AbortController[]; - let abortSignal: AbortSignal; - let handleAbort: jest.Mock; - - beforeEach(() => { - abortControllers = new Array(3).fill(undefined).map(() => new AbortController()); - handleAbort = jest.fn(); - - abortSignal = mergeAbortSignal(...abortControllers.map(({ signal }) => signal)); - abortSignal.addEventListener('abort', handleAbort); - }); - - test('should not abort initially', () => expect(abortSignal).toHaveProperty('aborted', false)); - test('should not receive "abort" event initially', () => expect(handleAbort).toBeCalledTimes(0)); - - describe.each([ - ['first', 0], - ['second', 1], - ['third', 2] - ])('when %s signal is aborted', (_, index) => { - beforeEach(() => abortControllers[index].abort()); - - test('should abort the signal', () => expect(abortSignal).toHaveProperty('aborted', true)); - test('should receive "abort" event', () => expect(handleAbort).toBeCalledTimes(1)); - - describe('when all signals are aborted', () => { - beforeEach(() => abortControllers.forEach(abortController => abortController.abort())); - - test('should abort the signal', () => expect(abortSignal).toHaveProperty('aborted', true)); - test('should receive "abort" event once', () => expect(handleAbort).toBeCalledTimes(1)); - }); - }); -}); diff --git a/src/streaming/mergeAbortSignal.ts b/src/streaming/mergeAbortSignal.ts deleted file mode 100644 index f24971d2..00000000 --- a/src/streaming/mergeAbortSignal.ts +++ /dev/null @@ -1,19 +0,0 @@ -// TODO: Add tests -export default function mergeAbortSignal(...signals: AbortSignal[]): AbortSignal { - const abortController = new AbortController(); - let handleAbort: () => void; - - handleAbort = () => { - abortController.abort(); - - for (const signal of signals) { - signal.removeEventListener('abort', handleAbort); - } - }; - - for (const signal of signals) { - signal.addEventListener('abort', handleAbort, { once: true }); - } - - return abortController.signal; -} diff --git a/src/streaming/sleep.spec.ts b/src/streaming/sleep.spec.ts deleted file mode 100644 index 883f1c51..00000000 --- a/src/streaming/sleep.spec.ts +++ /dev/null @@ -1,73 +0,0 @@ -import sleep from './sleep'; - -beforeEach(() => { - jest.useFakeTimers({ now: 0 }); -}); - -describe('sleep for 1s', () => { - let abortController: AbortController; - let now: number; - let promise: Promise; - - beforeEach(() => { - abortController = new AbortController(); - now = Date.now(); - promise = sleep(1000, { signal: abortController.signal }); - }); - - afterEach(() => { - promise.catch(() => {}); - }); - - test('should have 1 timer', () => expect(jest.getTimerCount()).toBe(1)); - - describe('when wake up', () => { - beforeEach(() => { - jest.runAllTimers(); - - return promise; - }); - - test('should passed 1s', () => expect(Date.now() - now).toBe(1000)); - }); - - describe('when aborted', () => { - beforeEach(() => abortController.abort()); - - test('should have no timers', () => expect(jest.getTimerCount()).toBe(0)); - test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); - }); - - describe('when 0.5s passed followed by abort', () => { - beforeEach(() => { - jest.advanceTimersByTime(500); - abortController.abort(); - }); - - test('should have no timers', () => expect(jest.getTimerCount()).toBe(0)); - test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); - }); -}); - -describe('abort before sleep for 1s', () => { - let abortController: AbortController; - let now: number; - let promise: Promise; - - beforeEach(() => { - abortController = new AbortController(); - abortController.abort(); - now = Date.now(); - promise = sleep(1000, { signal: abortController.signal }); - - jest.runAllTimers(); - }); - - afterEach(() => { - promise.catch(() => {}); - }); - - test('should have no timer', () => expect(jest.getTimerCount()).toBe(0)); - test('should not advance time', () => expect(Date.now()).toBe(now)); - test('should reject', () => expect(() => promise).rejects.toThrow('Aborted.')); -}); diff --git a/src/streaming/sleep.ts b/src/streaming/sleep.ts deleted file mode 100644 index 1406367c..00000000 --- a/src/streaming/sleep.ts +++ /dev/null @@ -1,27 +0,0 @@ -type Init = { - signal?: AbortSignal; -}; - -export default async function sleep(durationInMS: number = 100, init: Init = {}): Promise { - // Rectifying `durationInMS`. - durationInMS = Math.max(0, durationInMS); - - const { signal } = init; - - if (signal?.aborted) { - return Promise.reject(new Error('Aborted.')); - } - - let handleAbort: () => void; - let timeout: ReturnType; - - return new Promise((resolve, reject) => { - const handleAbort = () => reject(new Error('Aborted.')); - - timeout = setTimeout(resolve, durationInMS); - signal?.addEventListener('abort', handleAbort, { once: true }); - }).finally(() => { - clearTimeout(timeout); - signal?.removeEventListener('abort', handleAbort); - }); -} From 2fa1364adf4e332c00651c390e9d61020b5508b2 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 01:38:08 -0700 Subject: [PATCH 27/33] Update SSE --- README.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a75370be..5042930a 100644 --- a/README.md +++ b/README.md @@ -34,15 +34,19 @@ That said, the public API is still subject to change. ### Why the library did not detect Web Socket disconnections? -On iOS/iPadOS, when network change from Wi-Fi to cellular (and vice versa), the `WebSocket` object will be stalled without any errors. This is not detectable nor workaroundable without any additional assistance. The issue is related to an experimental feature named "NSURLSession WebSocket". The feature is enabled by default on iOS/iPadOS 15 and up. +On iOS/iPadOS, when network change from Wi-Fi to cellular, the `WebSocket` object will be stalled without any errors. This is not detectable nor workaroundable without any additional assistance. The issue is related to an experimental feature named "NSURLSession WebSocket". The feature is enabled by default on iOS/iPadOS 15 and up. -Web developers can use an option named `networkProbe` to assist the library to detect any connection issues. +An option named `networkInformation` can be used to assist the library to detect any connection issues. The option is based on [W3C Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) and it should implement at least 2 members: -One effective method to detect connection issues is establishing a long-polling HTTP GET call to any service. The service would respond with HTTP 2xx and stream 1 byte of data using [chunked transfer encoding](https://en.wikipedia.org/wiki/Chunked_transfer_encoding). After the first chunk, the streaming connection should be kept on-hold for 30 seconds. Then, the service will send a final chunk and end the call gracefully. If network change did happen during the call, the streaming conection will be aborted prematurely and the probe will send a fault signal to the library. +- [A `type` property](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type) to indicate the current network type + - When the `type` is `"offline"`, network is not available and no connection will be made +- [A `change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event) should dispatch when the `type` property change -If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). Upon receiving the event, the probe will send a fault signal to the library. +However, Safari on [does not support W3C Network Information API](https://bugs.webkit.org/show_bug.cgi?id=185697). It is up to web developers to implement the `NetworkInformation` polyfill. -Lastly, web developers can also implement custom connection detection mechanism using [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal). When connection issues is detected, the `AbortSignal` will be aborted and the probe will signal a fault. +One effective method to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network change did happen, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. Browser would automatically retries the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. + +If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should update the [`type` property](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type) based on network type and dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). ## How to build from source From 5462abc2fcdcc848ea193d2a7eb319d4f8150658 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 01:47:35 -0700 Subject: [PATCH 28/33] Update entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39ad7588..366cf3cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added -- Direct Line Streaming: Added `networkInformation` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#XXX](https://github.com/microsoft/BotFramework-DirectLineJS/pull/XXX) +- Direct Line Streaming: Added `networkInformation` option to assist detection of connection issues, by [@compulim](https://github.com/compulim), in PR [#412](https://github.com/microsoft/BotFramework-DirectLineJS/pull/412) ## [0.15.4] - 2023-06-05 From 0f7a7f2d18b300ce1949030d7a3b7dbd787285e1 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 02:00:53 -0700 Subject: [PATCH 29/33] Add clarifications --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5042930a..1df81b0f 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ An option named `networkInformation` can be used to assist the library to detect However, Safari on [does not support W3C Network Information API](https://bugs.webkit.org/show_bug.cgi?id=185697). It is up to web developers to implement the `NetworkInformation` polyfill. -One effective method to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network change did happen, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. Browser would automatically retries the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. +One effective way to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network type changed and current network type is no longer available, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. Browser would automatically retries the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should update the [`type` property](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type) based on network type and dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). From 00df4496f8a1db395ed9532fdfd5708d9455004f Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 13:39:32 -0700 Subject: [PATCH 30/33] Update README.md Co-authored-by: TJ Durnford --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 1df81b0f..0e0b48a7 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ An option named `networkInformation` can be used to assist the library to detect - When the `type` is `"offline"`, network is not available and no connection will be made - [A `change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event) should dispatch when the `type` property change -However, Safari on [does not support W3C Network Information API](https://bugs.webkit.org/show_bug.cgi?id=185697). It is up to web developers to implement the `NetworkInformation` polyfill. +However, Safari on iOS/iPadOS [does not support W3C Network Information API](https://bugs.webkit.org/show_bug.cgi?id=185697). It is up to web developers to implement the `NetworkInformation` polyfill. One effective way to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network type changed and current network type is no longer available, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. Browser would automatically retries the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. From 1dc55ae4bc78f5cdf03740249d5b9b578acb606d Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 02:27:41 -0700 Subject: [PATCH 31/33] Typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0e0b48a7..2a9bc412 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ An option named `networkInformation` can be used to assist the library to detect However, Safari on iOS/iPadOS [does not support W3C Network Information API](https://bugs.webkit.org/show_bug.cgi?id=185697). It is up to web developers to implement the `NetworkInformation` polyfill. -One effective way to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network type changed and current network type is no longer available, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. Browser would automatically retries the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. +One effective way to detect network type change is to subscribe to a [Server-Sent Events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) source. The service would send a message every 30 seconds. If network type changed and current network type is no longer available, the connection will be closed prematurely and an `error` event will be dispatched to the [`EventSource`](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) instance. Upon receiving the `error` event, the `NetworkInformation.type` should then change to `"offline"`. The browser would automatically retry the Server-Sent Events connection. Upon receiving an `open` event, the polyfill should change the `type` back to `"unknown"`. If the library is being used in a native iOS/iPadOS app, a less resource-intensive solution would be partially implementing the [Network Information API](https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API) using [`NWPathMonitor`](https://developer.apple.com/documentation/network/nwpathmonitor). When network change happens, the `NetworkInformation` instance should update the [`type` property](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type) based on network type and dispatch a [`change` event](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/change_event). From 6c40e6567569629f59c16fb55f930f90ac9f6a16 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 02:32:01 -0700 Subject: [PATCH 32/33] Update to Node.js 18 --- .github/workflows/continuous-deployment.yml | 4 ++-- .github/workflows/publish-release.yml | 6 +++--- .github/workflows/pull-request-validation.yml | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/continuous-deployment.yml b/.github/workflows/continuous-deployment.yml index 079d681e..4032961f 100644 --- a/.github/workflows/continuous-deployment.yml +++ b/.github/workflows/continuous-deployment.yml @@ -12,7 +12,7 @@ jobs: strategy: matrix: - node-version: [16.x] + node-version: [18.x] steps: - uses: actions/checkout@v3 @@ -48,7 +48,7 @@ jobs: steps: - uses: actions/setup-node@v3 with: - node-version: 16 + node-version: 18 registry-url: https://registry.npmjs.org/ - name: Download tarball artifact uses: actions/download-artifact@v3.0.1 diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 5af58a9c..87ab388f 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -10,10 +10,10 @@ jobs: steps: - uses: actions/checkout@v3 - - name: Use Node.js 16 + - name: Use Node.js 18 uses: actions/setup-node@v3 with: - node-version: 16 + node-version: 18 cache: 'npm' - id: get-version name: Get version @@ -49,7 +49,7 @@ jobs: steps: - uses: actions/setup-node@v3 with: - node-version: 16 + node-version: 18 registry-url: https://registry.npmjs.org/ - name: Download tarball artifact uses: actions/download-artifact@v3.0.1 diff --git a/.github/workflows/pull-request-validation.yml b/.github/workflows/pull-request-validation.yml index c96acf60..bdf3dea3 100644 --- a/.github/workflows/pull-request-validation.yml +++ b/.github/workflows/pull-request-validation.yml @@ -12,7 +12,7 @@ jobs: strategy: matrix: - node-version: [16.x] + node-version: [18.x] steps: - uses: actions/checkout@v3 From 1dd013ddb56aa332dab27a4a522fb126ca81de81 Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 11 Jul 2023 02:58:21 -0700 Subject: [PATCH 33/33] Add NetworkInformation.d.ts --- src/streaming/NetworkInformation.d.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 src/streaming/NetworkInformation.d.ts diff --git a/src/streaming/NetworkInformation.d.ts b/src/streaming/NetworkInformation.d.ts new file mode 100644 index 00000000..12c6e96f --- /dev/null +++ b/src/streaming/NetworkInformation.d.ts @@ -0,0 +1,24 @@ +declare global { + // This is subset of https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation. + interface NetworkInformation extends EventTarget { + addEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + + removeEventListener( + type: 'change', + listener: EventListener | EventListenerObject, + options?: AddEventListenerOptions | boolean + ): void; + + get type(): 'bluetooth' | 'cellular' | 'ethernet' | 'none' | 'other' | 'unknown' | 'wifi' | 'wimax'; + } + + interface Navigator { + get connection(): NetworkInformation; + } +} + +export {}