Rework connect saga to support connectivity status and reconnection (#…

…1649)

* Rework connect saga

* Wait for DISCONNECT_PENDING instead

* Add disconnect button

* Fix websocket param

* Add entry

* Remove dead code

* Remove console log

* Clean up

* Use ES form of subscribe

* Update PR number

* Bump botframework-webchat@0.11.0

* Bump botframework-directlinejs@0.11.0

* Use ConnectionStatus from DirectLineJS

* Vertically format if statement

* Remove try-catch

* botframework-directlinejs@^0.11.0

CHANGELOG.md

@@ -63,6 +63,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `bundle`: Fix [#1613](https://github.com/Microsoft/BotFramework-WebChat/issues/1613). Pass conversationId to DirectLineJS constructor, by [@neetu-das](https://github.com/neetu-das) in PR [#1614](https://github.com/Microsoft/BotFramework-WebChat/pull/1614)
 - `component`: Fix [#1626](https://github.com/Microsoft/BotFramework-WebChat/issues/1626). Fixed `Number.isNaN` is not available in IE11, by [@compulim](https://github.com/compulim) in PR [#1628](https://github.com/Microsoft/BotFramework-WebChat/pull/1628)
 - `bundle`: Fix [#1652](https://github.com/Microsoft/BotFramework-WebChat/issues/1652). Pass `pollingInterval` to DirectLineJS constructor, by [@neetu-das](https://github.com/neetu-das) in PR [#1655](https://github.com/Microsoft/BotFramework-WebChat/pull/1655)
+- `core`: Reworked logic on connect/disconnect for reliability on handling corner cases, by [@compulim](https://github.com/compulim) in PR [#1649](https://github.com/Microsoft/BotFramework-WebChat/pull/1649)
 
 ### Removed
 - `botAvatarImage` and `userAvatarImage` props, as they are moved inside `styleOptions`, in PR [#1486](https://github.com/Microsoft/BotFramework-WebChat/pull/1486)

packages/bundle/package.json

@@ -34,7 +34,7 @@
   "dependencies": {
     "@babel/runtime": "^7.1.2",
     "adaptivecards": "~1.1.2",
-    "botframework-directlinejs": "^0.10.2",
+    "botframework-directlinejs": "^0.11.0",
     "botframework-webchat-component": "^0.0.0-0",
     "botframework-webchat-core": "^0.0.0-0",
     "core-js": "^2.5.7",

packages/component/package.json

@@ -52,7 +52,7 @@
     "babel-jest": "^23.6.0",
     "babel-plugin-istanbul": "^5.1.0",
     "babel-plugin-version-transform": "^1.0.0",
-    "botframework-directlinejs": "^0.10.2",
+    "botframework-directlinejs": "^0.11.0",
     "concurrently": "^4.0.1",
     "jest": "^23.6.0",
     "react": "^16.4.1",
@@ -77,7 +77,7 @@
     "simple-update-in": "^1.3.0"
   },
   "peerDependencies": {
-    "botframework-directlinejs": "^0.10.2",
+    "botframework-directlinejs": "^0.11.0",
     "react": "^16.4.1"
   }
 }

packages/component/src/BasicWebChat.js

@@ -1,7 +1,5 @@
-import { createStore } from 'botframework-webchat-core';
 import { css } from 'glamor';
 import classNames from 'classnames';
-import memoize from 'memoize-one';
 import React from 'react';
 
 import BasicSendBox from './BasicSendBox';
@@ -30,7 +28,6 @@ export default class extends React.Component {
   constructor(props) {
     super(props);
 
-    this.createMemoizedStore = memoize(() => createStore());
     this.sendBoxRef = React.createRef();
 
     this.state = {

packages/core/package.json

@@ -50,7 +50,7 @@
     "babel-jest": "^23.6.0",
     "babel-plugin-istanbul": "^5.1.0",
     "babel-plugin-version-transform": "^1.0.0",
-    "botframework-directlinejs": "^0.10.2",
+    "botframework-directlinejs": "^0.11.0",
     "concurrently": "^4.0.1",
     "jest": "^23.6.0",
     "rimraf": "^2.6.2",

packages/core/src/actions/updateConnectionStatus.js

@@ -0,0 +1,10 @@
+const UPDATE_CONNECTION_STATUS = 'DIRECT_LINE/UPDATE_CONNECTION_STATUS';
+
+export default function updateConnectionStatus(connectionStatus) {
+  return {
+    type: UPDATE_CONNECTION_STATUS,
+    payload: { connectionStatus }
+  };
+}
+
+export { UPDATE_CONNECTION_STATUS }

packages/core/src/sagas/connectSaga.js

@@ -10,11 +10,12 @@ import {
 import { decode } from 'jsonwebtoken';
 import random from 'math-random';
 
-import callUntil from './effects/callUntil';
-import forever from './effects/forever';
+import updateConnectionStatus, { UPDATE_CONNECTION_STATUS } from '../actions/updateConnectionStatus';
 
 import createPromiseQueue from '../createPromiseQueue';
 
+import { ConnectionStatus } from 'botframework-directlinejs';
+
 import {
   CONNECT,
   CONNECT_PENDING,
@@ -26,108 +27,159 @@ import {
 import {
   DISCONNECT,
   DISCONNECT_PENDING,
-  DISCONNECT_REJECTED,
   DISCONNECT_FULFILLED
 } from '../actions/disconnect';
 
-// const UNINITIALIZED = 0;
-// const CONNECTING = 1;
-const ONLINE = 2;
-// const EXPIRED_TOKEN = 3;
-// const FAILED_TO_CONNECT = 4;
-const ENDED = 5;
+const {
+  Connecting: CONNECTING,
+  Online: ONLINE,
+  ExpiredToken: EXPIRED_TOKEN,
+  FailedToConnect: FAILED_TO_CONNECT,
+  Ended: ENDED
+} = ConnectionStatus;
 
 function randomUserID() {
   return `r_${ random().toString(36).substr(2, 10) }`;
 }
 
-export default function* () {
-  for (;;) {
-    const { payload: { directLine, userID } } = yield take(CONNECT);
-    const { token } = directLine;
-    const { user: userIDFromToken } = decode(token) || {};
+function* observeAndPutConnectionStatusUpdate(directLine) {
+  const connectionStatusQueue = createPromiseQueue();
+  const connectionStatusSubscription = directLine.connectionStatus$.subscribe({
+    next: connectionStatusQueue.push
+  });
 
-    if (userIDFromToken) {
-      if (userID && userID !== userIDFromToken) {
-        console.warn('Web Chat: user ID is both specified in the Direct Line token and passed in, will use the user ID from the token.');
-      }
+  try {
+    for (;;) {
+      const connectionStatus = yield call(connectionStatusQueue.shift);
 
-      userID = userIDFromToken;
-    } else if (userID) {
-      if (typeof userID !== 'string') {
-        console.warn('Web Chat: user ID must be a string.');
-        userID = randomUserID();
-      } else if (/^dl_/.test(userID)) {
-        console.warn('Web Chat: user ID prefixed with "dl_" is reserved and must be embedded into the Direct Line token to prevent forgery.');
-        userID = randomUserID();
-      }
-    } else {
-      // Only specify "default-user" if not found from token and not passed in
-      userID = randomUserID();
+      yield put(updateConnectionStatus(connectionStatus));
     }
+  } finally {
+    connectionStatusSubscription.unsubscribe();
+  }
+}
 
-    const connectTask = yield fork(connectSaga, directLine, userID);
+function negativeUpdateConnectionStatusAction({ payload, type }) {
+  if (type === UPDATE_CONNECTION_STATUS) {
+    const { connectionStatus } = payload;
 
-    yield take(DISCONNECT);
-    yield call(disconnectSaga, connectTask, directLine);
+    return (
+      connectionStatus !== CONNECTING
+      && connectionStatus !== ONLINE
+    );
   }
 }
 
-function* connectSaga(directLine, userID) {
-  const meta = { userID };
+function rectifyUserID(directLine, userIDFromAction) {
+  const { token } = directLine;
+  const { user: userIDFromToken } = decode(token) || {};
 
-  yield put({ type: CONNECT_PENDING, meta });
+  if (userIDFromToken) {
+    if (userIDFromAction && userIDFromAction !== userIDFromToken) {
+      console.warn('Web Chat: user ID is both specified in the Direct Line token and passed in, will use the user ID from the token.');
+    }
 
-  const connectionStatusQueue = createPromiseQueue();
-  const connectionStatusSubscription = directLine.connectionStatus$.subscribe({ next: connectionStatusQueue.push });
+    return userIDFromToken;
+  } else if (userIDFromAction) {
+    if (typeof userIDFromAction !== 'string') {
+      console.warn('Web Chat: user ID must be a string.');
+
+      return randomUserID();
+    } else if (/^dl_/.test(userIDFromAction)) {
+      console.warn('Web Chat: user ID prefixed with "dl_" is reserved and must be embedded into the Direct Line token to prevent forgery.');
+
+      return randomUserID();
+    }
+  } else {
+    return randomUserID();
+  }
 
+  return userIDFromAction;
+}
+
+function* connectSaga(directLine) {
   // DirectLineJS start the connection only after the first subscriber for activity$, but not connectionStatus$
   const activitySubscription = directLine.activity$.subscribe({ next: () => 0 });
 
   try {
-    try {
-      yield callUntil(connectionStatusQueue.shift, [], connectionStatus => connectionStatus === ONLINE);
-      yield put({ type: CONNECT_FULFILLING, meta, payload: { directLine } });
-      yield put({ type: CONNECT_FULFILLED, meta, payload: { directLine } });
-    } catch (err) {
-      yield put({ type: CONNECT_REJECTED, error: true, meta, payload: err });
-    } finally {
-      if (yield cancelled()) {
-        yield put({ type: CONNECT_REJECTED, error: true, meta, payload: new Error('cancelled') });
+    for (;;) {
+      const { payload: { connectionStatus } } = yield take(UPDATE_CONNECTION_STATUS);
+
+      // We will ignore DISCONNECT actions until we connect
+
+      if (connectionStatus === ONLINE) {
+        // TODO: [P2] DirectLineJS should kill the connection when we unsubscribe
+        //       But currently in v3, DirectLineJS does not have this functionality
+        //       Thus, we need to call "end()" explicitly
+
+        return () => {
+          activitySubscription.unsubscribe();
+          directLine.end();
+        };
+      } else if (
+        connectionStatus === ENDED
+        || connectionStatus === EXPIRED_TOKEN
+        || connectionStatus === FAILED_TO_CONNECT
+      ) {
+        // If we receive anything negative, we will assume the connection is errored out
+        throw new Error('Failed to connect');
       }
     }
-
-    yield forever();
   } finally {
-    // TODO: [P2] DirectLineJS should kill the connection when we unsubscribe
-    //       But currently in v3, DirectLineJS does not have this functionality
-    //       Thus, we need to call "end()" explicitly
-    directLine.end();
-    activitySubscription.unsubscribe();
-    connectionStatusSubscription.unsubscribe();
+    if (yield cancelled()) {
+      activitySubscription.unsubscribe();
+
+      throw new Error('Cancelled');
+    }
   }
 }
 
-function* disconnectSaga(connectTask, directLine) {
-  yield put({ type: DISCONNECT_PENDING });
+export default function* () {
+  for (;;) {
+    const { payload: { directLine, userID: userIDFromAction } } = yield take(CONNECT);
+    const updateConnectionStatusTask = yield fork(observeAndPutConnectionStatusUpdate, directLine);
 
-  const connectionStatusQueue = createPromiseQueue();
-  const unsubscribe = directLine.connectionStatus$.subscribe({ next: connectionStatusQueue.push });
+    try {
+      const meta = { userID: rectifyUserID(directLine, userIDFromAction) };
+      let endDirectLine;
 
-  // DirectLineJS should cancel underlying REST/WS when we cancel
-  // the connect task, which subsequently unsubscribe connectionStatus$
-  yield cancel(connectTask);
+      yield put({ type: CONNECT_PENDING, meta });
 
-  try {
-    yield callUntil(connectionStatusQueue.shift, [], connectionStatus => connectionStatus === ENDED);
-    yield put({ type: DISCONNECT_FULFILLED });
-  } catch (err) {
-    yield put({ type: DISCONNECT_REJECTED, error: true, payload: err });
-  } finally {
-    if (yield cancelled()) {
-      yield put({ type: DISCONNECT_REJECTED, error: true, payload: new Error('cancelled') });
-    }
+      try {
+        endDirectLine = yield call(connectSaga, directLine);
+      } catch (err) {
+        yield put({ type: CONNECT_REJECTED, error: true, meta, payload: err });
+
+        continue;
+      }
+
+      // At this point, we established connection to Direct Line.
+      // Any errors from this point, we need to make sure we call endDirectLine() to release resources.
+      try {
+        yield put({ type: CONNECT_FULFILLING, meta, payload: { directLine } });
+        yield put({ type: CONNECT_FULFILLED, meta, payload: { directLine } });
+
+        const terminateAction = yield take([DISCONNECT, negativeUpdateConnectionStatusAction]);
+
+        // Even if the connection is interrupted, we will still emitting DISCONNECT_PENDING.
+        // This will makes handling logic easier. If CONNECT_FULFILLED, we guarantee DISCONNECT_PENDING.
+        yield put({ type: DISCONNECT_PENDING });
 
-    unsubscribe();
+        endDirectLine();
+
+        if (terminateAction.type === DISCONNECT) {
+          // For graceful disconnect, we wait until Direct Line say it is ended
+          yield take(negativeUpdateConnectionStatusAction);
+        }
+      } finally {
+        // It is meaningless to continue to use the Direct Line object even disconnect failed.
+        // We will still unsubscribe to incoming activities and consider Direct Line object abandoned.
+        yield put({ type: DISCONNECT_FULFILLED });
+
+        endDirectLine();
+      }
+    } finally {
+      yield cancel(updateConnectionStatusTask);
+    }
   }
 }

packages/core/src/sagas/effects/whileConnected.js

@@ -6,15 +6,16 @@ import {
 } from 'redux-saga/effects';
 
 import { CONNECT_FULFILLING } from '../../actions/connect';
-import { DISCONNECT_FULFILLED } from '../../actions/disconnect';
+import { DISCONNECT_PENDING } from '../../actions/disconnect';
 
 export default function (fn) {
   return call(function* () {
     for (;;) {
       const { meta: { userID }, payload: { directLine } } = yield take(CONNECT_FULFILLING);
       const task = yield fork(fn, directLine, userID);
 
-      yield take(DISCONNECT_FULFILLED);
+      // When we receive DISCONNECT_PENDING, the Direct Line connection is tearing down and should not be used.
+      yield take(DISCONNECT_PENDING);
       yield cancel(task);
     }
   });

packages/playground/package.json

@@ -10,7 +10,7 @@
   ],
   "dependencies": {
     "adaptivecards": "~1.1.2",
-    "botframework-directlinejs": "^0.10.2",
+    "botframework-directlinejs": "^0.11.0",
     "botframework-webchat": "^0.0.0-0",
     "botframework-webchat-component": "^0.0.0-0",
     "botframework-webchat-core": "^0.0.0-0",