From a5538adf8a68270c1cdaceac32487b3521c25d54 Mon Sep 17 00:00:00 2001
From: Lyubo Marinov <lmarinov@atlassian.com>
Date: Tue, 5 Dec 2017 14:34:24 -0600
Subject: [PATCH] [RN] Add a timeout for loading the configuration

---
 react/features/app/actions.js                 |  7 +---
 .../features/base/lib-jitsi-meet/functions.js | 16 +++++----
 react/features/base/util/helpers.js           | 30 ++--------------
 react/features/base/util/loadScript.native.js | 35 ++++++++++++++++--
 react/features/base/util/loadScript.web.js    |  2 +-
 react/features/base/util/randomUtil.js        |  2 +-
 react/features/base/util/timeoutPromise.js    | 36 +++++++++++++++++++
 7 files changed, 84 insertions(+), 44 deletions(-)
 create mode 100644 react/features/base/util/timeoutPromise.js

diff --git a/react/features/app/actions.js b/react/features/app/actions.js
index 10be99372..edc03d079 100644
--- a/react/features/app/actions.js
+++ b/react/features/app/actions.js
@@ -10,11 +10,6 @@ import { APP_WILL_MOUNT, APP_WILL_UNMOUNT } from './actionTypes';
 
 declare var APP: Object;
 
-/**
- * Timeout for loading the configuration.
- */
-const LOAD_CONFIG_TIMEOUT = 8000;
-
 /**
  * Triggers an in-app navigation to a specific route. Allows navigation to be
  * abstracted between the mobile/React Native and Web/React applications.
@@ -211,7 +206,7 @@ function _loadConfig({ contextRoot, host, protocol, room }) {
 
     const key = `config.js/${baseURL}`;
 
-    return loadConfig(url, LOAD_CONFIG_TIMEOUT).then(
+    return loadConfig(url).then(
         /* onFulfilled */ config => {
             // Try to store the configuration in localStorage. If the deployment
             // specified 'getroom' as a function, for example, it does not make
diff --git a/react/features/base/lib-jitsi-meet/functions.js b/react/features/base/lib-jitsi-meet/functions.js
index 0707b2946..caafacdfe 100644
--- a/react/features/base/lib-jitsi-meet/functions.js
+++ b/react/features/base/lib-jitsi-meet/functions.js
@@ -2,7 +2,7 @@
 
 import { setConfigFromURLParams } from '../config';
 import { toState } from '../redux';
-import { loadScript, timeoutPromise } from '../util';
+import { loadScript } from '../util';
 
 import JitsiMeetJS from './_';
 
@@ -107,16 +107,20 @@ export function isFatalJitsiConnectionError(error: Object | string) {
  * Loads config.js from a specific remote server.
  *
  * @param {string} url - The URL to load.
- * @param {number} timeoutMs - The timeout for the configuration to be loaded,
- * in milliseconds.
+ * @param {number} [timeout] - The timeout for the configuration to be loaded,
+ * in milliseconds. If not specified, a default value deamed appropriate for the
+ * purpsoe is used.
  * @returns {Promise<Object>}
  */
-export function loadConfig(url: string, timeoutMs: number): Promise<Object> {
+export function loadConfig(
+        url: string,
+        timeout: ?number = 10 /* seconds */ * 1000 /* in milliseconds */
+): Promise<Object> {
     let promise;
 
     if (typeof APP === 'undefined') {
         promise
-            = loadScript(url)
+            = loadScript(url, timeout)
                 .then(() => {
                     const { config } = window;
 
@@ -150,5 +154,5 @@ export function loadConfig(url: string, timeoutMs: number): Promise<Object> {
         return value;
     });
 
-    return timeoutPromise(promise, timeoutMs);
+    return promise;
 }
diff --git a/react/features/base/util/helpers.js b/react/features/base/util/helpers.js
index e151ef2b7..8cdbbedfe 100644
--- a/react/features/base/util/helpers.js
+++ b/react/features/base/util/helpers.js
@@ -1,3 +1,5 @@
+// @flow
+
 /**
  * Returns the namespace for all global variables, functions, etc that we need.
  *
@@ -16,31 +18,3 @@ export function getJitsiMeetGlobalNS() {
 
     return window.JitsiMeetJS.app;
 }
-
-/**
- * Makes the given promise fail with a timeout error if it wasn't fulfilled in
- * the given timeout.
- *
- * @param {Promise} promise - The promise which will be wrapped for timeout.
- * @param {number} ms - The amount of milliseconds to wait for a response before
- * failing with a timeout error.
- * @returns {Promise} - The wrapped promise.
- */
-export function timeoutPromise(promise, ms) {
-    return new Promise((resolve, reject) => {
-        const timeoutId = setTimeout(() => {
-            reject(new Error('timeout'));
-        }, ms);
-
-        promise.then(
-            res => {
-                clearTimeout(timeoutId);
-                resolve(res);
-            },
-            err => {
-                clearTimeout(timeoutId);
-                reject(err);
-            }
-        );
-    });
-}
diff --git a/react/features/base/util/loadScript.native.js b/react/features/base/util/loadScript.native.js
index cd24925a4..fbaa6e8c7 100644
--- a/react/features/base/util/loadScript.native.js
+++ b/react/features/base/util/loadScript.native.js
@@ -1,3 +1,7 @@
+// @flow
+
+import { timeoutPromise } from './timeoutPromise';
+
 /**
  * Loads a script from a specific URL. React Native cannot load a JS
  * file/resource/URL via a <script> HTML element, so the implementation
@@ -6,9 +10,12 @@
  *
  * @param {string} url - The absolute URL from which the script is to be
  * (down)loaded.
+ * @param {number} [timeout] - The timeout in millisecnods after which the
+ * loading of the specified {@code url} is to be aborted/rejected (if not
+ * settled yet).
  * @returns {void}
  */
-export function loadScript(url) {
+export function loadScript(url: string, timeout: ?number): Promise<void> {
     return new Promise((resolve, reject) => {
         // XXX The implementation of fetch on Android will throw an Exception on
         // the Java side which will break the app if the URL is invalid (which
@@ -26,7 +33,31 @@ export function loadScript(url) {
             return;
         }
 
-        fetch(url, { method: 'GET' })
+        let fetch_ = fetch(url, { method: 'GET' });
+
+        // The implementation of fetch provided by react-native is based on
+        // XMLHttpRequest. Which defines timeout as an unsigned long with
+        // default value 0, which means there is no timeout.
+        if (timeout) {
+            // FIXME I don't like the approach with timeoutPromise because:
+            //
+            // * It merely abandons the underlying XHR and, consequently, opens
+            //   us to potential issues with NetworkActivityIndicator which
+            //   tracks XHRs.
+            //
+            // * @paweldomas also reported that timeouts seem to be respected by
+            //   the XHR implementation on iOS. Given that we have
+            //   implementation of loadScript based on fetch and XHR (in an
+            //   earlier revision), I don't see why we're not using an XHR
+            //   directly on iOS.
+            //
+            // * The approach of timeoutPromise I found on the Internet is to
+            //   directly use XHR instead of fetch and abort the XHR on timeout.
+            //   Which may deal with the NetworkActivityIndicator at least.
+            fetch_ = timeoutPromise(fetch_, timeout);
+        }
+
+        fetch_
             .then(response => {
                 switch (response.status) {
                 case 200:
diff --git a/react/features/base/util/loadScript.web.js b/react/features/base/util/loadScript.web.js
index 9ea262a3a..f03a9d77a 100644
--- a/react/features/base/util/loadScript.web.js
+++ b/react/features/base/util/loadScript.web.js
@@ -1,4 +1,4 @@
-/* @flow */
+// @flow
 
 declare var JitsiMeetJS: Object;
 
diff --git a/react/features/base/util/randomUtil.js b/react/features/base/util/randomUtil.js
index 8ad261bdd..a657ee8d2 100644
--- a/react/features/base/util/randomUtil.js
+++ b/react/features/base/util/randomUtil.js
@@ -1,4 +1,4 @@
-/* @flow */
+// @flow
 
 /**
  * Alphanumeric characters.
diff --git a/react/features/base/util/timeoutPromise.js b/react/features/base/util/timeoutPromise.js
new file mode 100644
index 000000000..5f45d031d
--- /dev/null
+++ b/react/features/base/util/timeoutPromise.js
@@ -0,0 +1,36 @@
+// @flow
+
+/**
+ * Returns a new {@code Promise} which settles when a specific {@code Promise}
+ * settles and is automatically rejected if the specified {@code Promise}
+ * doesn't settle within a specific time interval.
+ *
+ * @param {Promise} promise - The {@code Promise} for which automatic rejecting
+ * after the speicified timout is to be implemented.
+ * @param {number} timeout - The number of milliseconds to wait the specified
+ * {@code promise} to settle before automatically rejecting the returned
+ * {@code Promise}.
+ * @returns {Promise} - A new {@code Promise} which settles when the specified
+ * {@code promise} settles and is automatically rejected after {@code timeout}
+ * milliseconds.
+ */
+export function timeoutPromise<T>(
+        promise: Promise<T>,
+        timeout: number
+): Promise<T> {
+    return new Promise((resolve, reject) => {
+        const timeoutID
+            = setTimeout(() => reject(new Error('timeout')), timeout);
+
+        promise.then(
+            /* onFulfilled */ value => {
+                resolve(value);
+                clearTimeout(timeoutID);
+            },
+            /* onRejected */ reason => {
+                reject(reason);
+                clearTimeout(timeoutID);
+            }
+        );
+    });
+}