From 81ee18d3860d1a1c1a04525fbb9fb7f4f19f1129 Mon Sep 17 00:00:00 2001 From: William Wong Date: Fri, 12 Oct 2018 22:07:43 -0700 Subject: [PATCH 1/7] Do not poll if last polling was not complete --- src/directLine.ts | 90 +++++++++++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 35 deletions(-) diff --git a/src/directLine.ts b/src/directLine.ts index 947d8b2f6..095b1ee8f 100644 --- a/src/directLine.ts +++ b/src/directLine.ts @@ -279,7 +279,7 @@ const errorFailedToConnect = new Error("failed to connect"); const konsole = { log: (message?: any, ... optionalParams: any[]) => { - if (typeof(window) !== 'undefined' && (window as any)["botchatDebug"] && message) + if (typeof window !== 'undefined' && (window as any)["botchatDebug"] && message) console.log(message, ... optionalParams); } } @@ -319,15 +319,15 @@ export class DirectLine implements IBotConnection { if (options.domain) { this.domain = options.domain; } - + if (options.conversationId) { this.conversationId = options.conversationId; } - + if (options.watermark) { this.watermark = options.watermark; } - + if (options.streamUrl) { if (options.token && options.conversationId) { this.streamUrl = options.streamUrl; @@ -335,7 +335,7 @@ export class DirectLine implements IBotConnection { console.warn('streamUrl was ignored: you need to provide a token and a conversationid'); } } - + if (options.pollingInterval !== undefined) { this.pollingInterval = options.pollingInterval; } @@ -388,10 +388,10 @@ export class DirectLine implements IBotConnection { return Observable.throw(errorFailedToConnect); case ConnectionStatus.ExpiredToken: - return Observable.of(connectionStatus); + return Observable.throw(errorExpiredToken); default: - return Observable.of(connectionStatus); + return Observable.of(null); } }) @@ -461,7 +461,11 @@ export class DirectLine implements IBotConnection { // if the token is expired there's no reason to keep trying this.expiredToken(); return Observable.throw(error); + } else if (error.status === 404) { + // If the bot is gone, we should stop retrying + return Observable.throw(error); } + return Observable.of(error); }) .delay(timeout) @@ -600,37 +604,51 @@ export class DirectLine implements IBotConnection { } private pollingGetActivity$() { + // Skip if the last request is still pending + let shouldSkip = false; + return Observable.interval(this.pollingInterval) .combineLatest(this.checkConnection()) - .flatMap(([_, connectionStatus]) => { - if (connectionStatus !== ConnectionStatus.Online) - return Observable.empty() + .flatMap(() => { + if (shouldSkip) { + return Observable.empty(); + } else { + shouldSkip = true; - return Observable.ajax({ - method: "GET", - url: `${this.domain}/conversations/${this.conversationId}/activities?watermark=${this.watermark}`, - timeout, - headers: { - "Accept": "application/json", - "Authorization": `Bearer ${this.token}` - } - }) - .catch(error => { - if (error.status === 403) { - // This is slightly ugly. We want to update this.connectionStatus$ to ExpiredToken so that subsequent - // calls to checkConnection will throw an error. But when we do so, it causes this.checkConnection() - // to immediately throw an error, which is caught by the catch() below and transformed into an empty - // object. Then next() returns, and we emit an empty object. Which means one 403 is causing - // two empty objects to be emitted. Which is harmless but, again, slightly ugly. - this.expiredToken(); - } - return Observable.empty(); - }) -// .do(ajaxResponse => konsole.log("getActivityGroup ajaxResponse", ajaxResponse)) - .map(ajaxResponse => ajaxResponse.response as ActivityGroup) - .flatMap(activityGroup => this.observableFromActivityGroup(activityGroup)) + return Observable.ajax({ + method: "GET", + url: `${this.domain}/conversations/${this.conversationId}/activities?watermark=${this.watermark}`, + timeout, + headers: { + "Accept": "application/json", + "Authorization": `Bearer ${this.token}` + } + }) + .do(() => { + shouldSkip = false; + }, () => { + shouldSkip = false; + }) + .catch(error => { + if (error.status === 403) { + // This is slightly ugly. We want to update this.connectionStatus$ to ExpiredToken so that subsequent + // calls to checkConnection will throw an error. But when we do so, it causes this.checkConnection() + // to immediately throw an error, which is caught by the catch() below and transformed into an empty + // object. Then next() returns, and we emit an empty object. Which means one 403 is causing + // two empty objects to be emitted. Which is harmless but, again, slightly ugly. + this.expiredToken(); + } else if (error.status === 404) { + return Observable.throw(errorConversationEnded); + } + + return Observable.empty(); + }) + // .do(ajaxResponse => konsole.log("getActivityGroup ajaxResponse", ajaxResponse)) + .map(ajaxResponse => ajaxResponse.response as ActivityGroup) + .flatMap(activityGroup => this.observableFromActivityGroup(activityGroup)) + } }) - .catch(error => Observable.empty()); + .catch(() => Observable.empty()); } private observableFromActivityGroup(activityGroup: ActivityGroup) { @@ -711,7 +729,10 @@ export class DirectLine implements IBotConnection { // token has expired. We can't recover from this here, but the embedding // website might eventually call reconnect() with a new token and streamUrl. this.expiredToken(); + } else if (error.status === 404) { + return Observable.throw(errorConversationEnded); } + return Observable.of(error); }) .delay(timeout) @@ -719,5 +740,4 @@ export class DirectLine implements IBotConnection { ) ) } - } From 178f8f72ce9a5226cdfdf6102559dd20fc7a90bf Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 15 Oct 2018 16:25:50 -0700 Subject: [PATCH 2/7] Revert expired token check --- src/directLine.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/directLine.ts b/src/directLine.ts index 095b1ee8f..9c99db940 100644 --- a/src/directLine.ts +++ b/src/directLine.ts @@ -388,10 +388,10 @@ export class DirectLine implements IBotConnection { return Observable.throw(errorFailedToConnect); case ConnectionStatus.ExpiredToken: - return Observable.throw(errorExpiredToken); + return Observable.of(connectionStatus); default: - return Observable.of(null); + return Observable.of(connectionStatus); } }) From c0e2bab7f4cd901164174a84b07b7621fb9e21e5 Mon Sep 17 00:00:00 2001 From: William Wong Date: Mon, 15 Oct 2018 16:26:52 -0700 Subject: [PATCH 3/7] Bump to 0.10.0-0 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index f26ed78ba..91426fd08 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "botframework-directlinejs", - "version": "0.9.18-0", + "version": "0.10.0-0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index ef6feafac..4ee520779 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "botframework-directlinejs", - "version": "0.9.18-0", + "version": "0.10.0-0", "description": "client library for the Microsoft Bot Framework Direct Line 3.0 protocol", "main": "built/directLine.js", "types": "built/directLine.d.ts", From 632bb95358a54166d2640e70bed834a5486f1654 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 17 Oct 2018 10:35:06 -0700 Subject: [PATCH 4/7] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3c82ce35..63cd5faed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed - Delay before retrying Web Socket, in [#97](https://github.com/Microsoft/BotFramework-WebChat/pull/97) +- Slow down polling on congested traffic, in [#98](https://github.com/Microsoft/BotFramework-DirectLineJS/pull/98) ## [0.9.17] - 2018-08-31 ### Changed From 5c02e8341f95a0e508facaa4ef451de77dc4199f Mon Sep 17 00:00:00 2001 From: CK Kashyap Date: Thu, 18 Oct 2018 13:30:36 -0700 Subject: [PATCH 5/7] Fix for speedy retry on 403 --- src/directLine.ts | 73 +++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/src/directLine.ts b/src/directLine.ts index 9c99db940..99d820753 100644 --- a/src/directLine.ts +++ b/src/directLine.ts @@ -604,18 +604,12 @@ export class DirectLine implements IBotConnection { } private pollingGetActivity$() { - // Skip if the last request is still pending - let shouldSkip = false; - - return Observable.interval(this.pollingInterval) - .combineLatest(this.checkConnection()) - .flatMap(() => { - if (shouldSkip) { - return Observable.empty(); - } else { - shouldSkip = true; - - return Observable.ajax({ + var poller$ : Observable = Observable.create((theSubscriber:Subscriber) => { + // A BehaviorSubject to trigger polling. Since it is a BehaviorSubject + // the first event is produced immediately. + var trigger$ = new BehaviorSubject({}); + trigger$.subscribe((data)=> { + var ajax : Observable = Observable.ajax({ method: "GET", url: `${this.domain}/conversations/${this.conversationId}/activities?watermark=${this.watermark}`, timeout, @@ -623,32 +617,43 @@ export class DirectLine implements IBotConnection { "Accept": "application/json", "Authorization": `Bearer ${this.token}` } - }) - .do(() => { - shouldSkip = false; - }, () => { - shouldSkip = false; - }) - .catch(error => { + }); + + // Capture the polling interval and connectionStatus$ so that it + // may be used within callbacks that are not invoked with the + // object context + var pollingIntervalMS = this.pollingInterval; + var connectionStatus$ = this.connectionStatus$; + var startTimestampMS = Date.now() + let onSuccess = function (result:AjaxResponse) { + theSubscriber.next(result); + var interval = pollingIntervalMS - Date.now() + startTimestampMS; + setTimeout(() => trigger$.next(data), interval < 0 ? 0 : interval); + } + + let onError = function(error:any) { if (error.status === 403) { - // This is slightly ugly. We want to update this.connectionStatus$ to ExpiredToken so that subsequent - // calls to checkConnection will throw an error. But when we do so, it causes this.checkConnection() - // to immediately throw an error, which is caught by the catch() below and transformed into an empty - // object. Then next() returns, and we emit an empty object. Which means one 403 is causing - // two empty objects to be emitted. Which is harmless but, again, slightly ugly. - this.expiredToken(); + connectionStatus$.next(ConnectionStatus.ExpiredToken) + setTimeout(() => trigger$.next(data), pollingIntervalMS) } else if (error.status === 404) { - return Observable.throw(errorConversationEnded); + connectionStatus$.next(ConnectionStatus.Ended) + } else { + // propagate the error + theSubscriber.error(error) } + } - return Observable.empty(); - }) - // .do(ajaxResponse => konsole.log("getActivityGroup ajaxResponse", ajaxResponse)) - .map(ajaxResponse => ajaxResponse.response as ActivityGroup) - .flatMap(activityGroup => this.observableFromActivityGroup(activityGroup)) - } - }) - .catch(() => Observable.empty()); + if (this.connectionStatus$.getValue() === ConnectionStatus.Online) + ajax.subscribe(onSuccess, onError); + }); + }); + + return this.checkConnection() + .flatMap(_ => + poller$ + .catch(error => Observable.empty()) + .map(ajaxResponse => ajaxResponse.response as ActivityGroup) + .flatMap(activityGroup => this.observableFromActivityGroup(activityGroup))); } private observableFromActivityGroup(activityGroup: ActivityGroup) { From cea06cec7ea5d30217ce86b0d6190938d51e7e9d Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 23 Oct 2018 14:53:56 -0700 Subject: [PATCH 6/7] Update directLine.ts --- src/directLine.ts | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/directLine.ts b/src/directLine.ts index 99d820753..0b9183cc6 100644 --- a/src/directLine.ts +++ b/src/directLine.ts @@ -604,47 +604,44 @@ export class DirectLine implements IBotConnection { } private pollingGetActivity$() { - var poller$ : Observable = Observable.create((theSubscriber:Subscriber) => { + const poller$: Observable = Observable.create((theSubscriber: Subscriber) => { // A BehaviorSubject to trigger polling. Since it is a BehaviorSubject // the first event is produced immediately. - var trigger$ = new BehaviorSubject({}); - trigger$.subscribe((data)=> { - var ajax : Observable = Observable.ajax({ - method: "GET", - url: `${this.domain}/conversations/${this.conversationId}/activities?watermark=${this.watermark}`, + const trigger$ = new BehaviorSubject({}); + + trigger$.subscribe(data => { + const ajax: Observable = Observable.ajax({ + method: 'GET', + url: `${ this.domain }/conversations/${ this.conversationId }/activities?watermark=${ this.watermark }`, timeout, headers: { - "Accept": "application/json", - "Authorization": `Bearer ${this.token}` + Accept: 'application/json', + Authorization: `Bearer ${ this.token }` } }); - // Capture the polling interval and connectionStatus$ so that it - // may be used within callbacks that are not invoked with the - // object context - var pollingIntervalMS = this.pollingInterval; - var connectionStatus$ = this.connectionStatus$; - var startTimestampMS = Date.now() - let onSuccess = function (result:AjaxResponse) { + const startTimestampMS = Date.now(); + + let onSuccess = (result: AjaxResponse) => { theSubscriber.next(result); - var interval = pollingIntervalMS - Date.now() + startTimestampMS; - setTimeout(() => trigger$.next(data), interval < 0 ? 0 : interval); - } + setTimeout(() => trigger$.next(data), Math.max(0, this.pollingInterval - Date.now() + startTimestampMS)); + }; - let onError = function(error:any) { + let onError = (error: any) => { if (error.status === 403) { - connectionStatus$.next(ConnectionStatus.ExpiredToken) - setTimeout(() => trigger$.next(data), pollingIntervalMS) + this.connectionStatus$.next(ConnectionStatus.ExpiredToken); + setTimeout(() => trigger$.next(data), this.pollingIntervalMS); } else if (error.status === 404) { - connectionStatus$.next(ConnectionStatus.Ended) + this.connectionStatus$.next(ConnectionStatus.Ended); } else { // propagate the error - theSubscriber.error(error) + theSubscriber.error(error); } - } + }; - if (this.connectionStatus$.getValue() === ConnectionStatus.Online) + if (this.connectionStatus$.getValue() === ConnectionStatus.Online) { ajax.subscribe(onSuccess, onError); + } }); }); From d620ba9620d3fced232d37c081d16e6b01e6773d Mon Sep 17 00:00:00 2001 From: William Wong Date: Tue, 23 Oct 2018 15:14:18 -0700 Subject: [PATCH 7/7] Cleanup --- src/directLine.ts | 72 ++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/src/directLine.ts b/src/directLine.ts index 0b9183cc6..9750b71b6 100644 --- a/src/directLine.ts +++ b/src/directLine.ts @@ -604,51 +604,53 @@ export class DirectLine implements IBotConnection { } private pollingGetActivity$() { - const poller$: Observable = Observable.create((theSubscriber: Subscriber) => { + const poller$: Observable = Observable.create((subscriber: Subscriber) => { // A BehaviorSubject to trigger polling. Since it is a BehaviorSubject // the first event is produced immediately. const trigger$ = new BehaviorSubject({}); - trigger$.subscribe(data => { - const ajax: Observable = Observable.ajax({ - method: 'GET', - url: `${ this.domain }/conversations/${ this.conversationId }/activities?watermark=${ this.watermark }`, - timeout, - headers: { - Accept: 'application/json', - Authorization: `Bearer ${ this.token }` - } - }); - - const startTimestampMS = Date.now(); - - let onSuccess = (result: AjaxResponse) => { - theSubscriber.next(result); - setTimeout(() => trigger$.next(data), Math.max(0, this.pollingInterval - Date.now() + startTimestampMS)); - }; - - let onError = (error: any) => { - if (error.status === 403) { - this.connectionStatus$.next(ConnectionStatus.ExpiredToken); - setTimeout(() => trigger$.next(data), this.pollingIntervalMS); - } else if (error.status === 404) { - this.connectionStatus$.next(ConnectionStatus.Ended); - } else { - // propagate the error - theSubscriber.error(error); - } - }; - + trigger$.subscribe(() => { if (this.connectionStatus$.getValue() === ConnectionStatus.Online) { - ajax.subscribe(onSuccess, onError); + const startTimestamp = Date.now(); + + Observable.ajax({ + headers: { + Accept: 'application/json', + Authorization: `Bearer ${ this.token }` + }, + method: 'GET', + url: `${ this.domain }/conversations/${ this.conversationId }/activities?watermark=${ this.watermark }`, + timeout + }).subscribe( + (result: AjaxResponse) => { + subscriber.next(result); + setTimeout(() => trigger$.next(null), Math.max(0, this.pollingInterval - Date.now() + startTimestamp)); + }, + (error: any) => { + switch (error.status) { + case 403: + this.connectionStatus$.next(ConnectionStatus.ExpiredToken); + setTimeout(() => trigger$.next(null), this.pollingInterval); + break; + + case 404: + this.connectionStatus$.next(ConnectionStatus.Ended); + break; + + default: + // propagate the error + subscriber.error(error); + break; + } + } + ); } }); }); return this.checkConnection() - .flatMap(_ => - poller$ - .catch(error => Observable.empty()) + .flatMap(_ => poller$ + .catch(() => Observable.empty()) .map(ajaxResponse => ajaxResponse.response as ActivityGroup) .flatMap(activityGroup => this.observableFromActivityGroup(activityGroup))); }