ref(IceFailedHandling): use ping instead of networkInfo

Simplifies the logic for delayed ICE failed event by relying
on XMPP ping instead of the networkInfo module to check
if the internet is online.

modules/connectivity/IceFailedHandling.js

@@ -4,60 +4,8 @@ import { getLogger } from 'jitsi-meet-logger';
 import * as JitsiConferenceErrors from '../../JitsiConferenceErrors';
 import * as JitsiConferenceEvents from '../../JitsiConferenceEvents';
 
-import { default as networkInfo, NETWORK_INFO_EVENT } from './NetworkInfo';
-
 const logger = getLogger(__filename);
 
-/**
- * Helper class for handling ICE event delay in combination with internet online/offline status check.
- */
-class DelayedIceFailedEvent {
-    /**
-     * A constructor.
-     * @param {function} emitIceFailed - Will be called by this class to emit ICE failed conference event.
-     * @param {number} delay - The delay for ICE failed in milliseconds since the event occurred on the peerconnection
-     * or the internet came back online.
-     */
-    constructor(emitIceFailed, delay) {
-        this._emitIceFailed = emitIceFailed;
-        this._delay = delay;
-    }
-
-    /**
-     * Starts the event delay and internet status check logic.
-     */
-    start() {
-        this._onlineListener
-            = networkInfo.addEventListener(
-                NETWORK_INFO_EVENT,
-                () => this._maybeSetDelayTimeout());
-        this._maybeSetDelayTimeout();
-    }
-
-    /**
-     * Cancels the task.
-     */
-    stop() {
-        this._onlineListener && this._onlineListener();
-        this._onlineListener = undefined;
-        clearTimeout(this._delayTimeout);
-    }
-
-    /**
-     * Resets the timer delay if the internet status is online.
-     * @private
-     */
-    _maybeSetDelayTimeout() {
-        clearTimeout(this._delayTimeout);
-
-        if (networkInfo.isOnline()) {
-            logger.info(`Will emit ICE failed in ${this._delay}ms`);
-            this._delayTimeout = setTimeout(() => this._emitIceFailed(), this._delay);
-        }
-    }
-}
-
-
 /**
  * This class deals with shenanigans around JVB media session's ICE failed status handling.
  *
@@ -79,21 +27,38 @@ export default class IceFailedHandling {
     }
 
     /**
-     * Starts the task.
+     * After making sure there's no way for the ICE connection to recover this method either sends ICE failed
+     * notification to Jicofo or emits the ice failed conference event.
+     * @private
+     * @returns {void}
      */
-    start() {
+    _actOnIceFailed() {
         if (!this._conference.options.config.enableIceRestart) {
             logger.info('ICE failed, but ICE restarts are disabled');
-            this._delayedIceFailedEvent = new DelayedIceFailedEvent(() => {
-                this._conference.eventEmitter.emit(
-                    JitsiConferenceEvents.CONFERENCE_FAILED,
-                    JitsiConferenceErrors.ICE_FAILED);
-            }, 15000);
-            this._delayedIceFailedEvent.start();
+            this._conference.eventEmitter.emit(
+                JitsiConferenceEvents.CONFERENCE_FAILED,
+                JitsiConferenceErrors.ICE_FAILED);
 
             return;
         }
 
+        const jvbConnection = this._conference.jvbJingleSession;
+        const jvbConnIceState = jvbConnection && jvbConnection.getIceConnectionState();
+
+        if (!jvbConnection) {
+            logger.warn('Not sending ICE failed - no JVB connection');
+        } else if (jvbConnIceState === 'connected') {
+            logger.info('ICE connection restored - not sending ICE failed');
+        } else {
+            logger.info(`Sending ICE failed - the connection has not recovered: ${jvbConnIceState}`);
+            jvbConnection.sendIceFailedNotification();
+        }
+    }
+
+    /**
+     * Starts the task.
+     */
+    start() {
         //  Using xmpp.ping allows to handle both XMPP being disconnected and internet offline cases. The ping function
         // uses sendIQ2 method which is resilient to XMPP connection disconnected state and will patiently wait until it
         // gets reconnected.
@@ -105,22 +70,10 @@ export default class IceFailedHandling {
         // to 'item-not-found' error returned by the server.
         this._conference.xmpp.ping(65000).then(
             () => {
-                if (this._canceled) {
-                    return;
-                }
-
-                const jvbConnection = this._conference.jvbJingleSession;
-                const jvbConnIceState = jvbConnection && jvbConnection.getIceConnectionState();
-
-                if (!jvbConnection) {
-                    logger.warn('Not sending ICE failed - no JVB connection');
-                } else if (jvbConnIceState === 'connected') {
-                    logger.info('ICE connection restored - not sending ICE failed');
-                } else {
+                if (!this._canceled) {
                     this._iceFailedTimeout = window.setTimeout(() => {
-                        logger.info(`Sending ICE failed - the connection has not recovered: ${jvbConnIceState}`);
                         this._iceFailedTimeout = undefined;
-                        jvbConnection.sendIceFailedNotification();
+                        this._actOnIceFailed();
                     }, 2000);
                 }
             },
@@ -135,6 +88,5 @@ export default class IceFailedHandling {
     cancel() {
         this._canceled = true;
         window.clearTimeout(this._iceFailedTimeout);
-        this._delayedIceFailedEvent && this._delayedIceFailedEvent.stop();
     }
 }

modules/connectivity/IceFailedHandling.spec.js

@@ -4,7 +4,6 @@ import Listenable from '../util/Listenable';
 import { nextTick } from '../util/TestUtils';
 
 import IceFailedHandling from './IceFailedHandling';
-import networkInfo from './NetworkInfo';
 
 /**
  * Mock conference for the purpose of this test.
@@ -28,13 +27,15 @@ describe('IceFailedHandling', () => {
 
     beforeEach(() => {
         jasmine.clock().install();
-        networkInfo.updateNetworkInfo({ isOnline: true });
         mockConference = new MockConference();
         iceFailedHandling = new IceFailedHandling(mockConference);
         mockConference.eventEmitter = {
             // eslint-disable-next-line no-empty-function
             emit: () => { }
         };
+        mockConference.xmpp = {
+            ping: () => Promise.resolve()
+        };
         emitEventSpy = spyOn(mockConference.eventEmitter, 'emit');
     });
 
@@ -46,34 +47,32 @@ describe('IceFailedHandling', () => {
         beforeEach(() => {
             mockConference.options.config.enableIceRestart = false;
         });
-        it('emits ICE failed with 15 seconds delay', () => {
-            iceFailedHandling.start();
-            jasmine.clock().tick(10000);
-            expect(emitEventSpy).not.toHaveBeenCalled();
-
-            jasmine.clock().tick(5100);
-            expect(emitEventSpy).toHaveBeenCalled();
-        });
-        it('starts counting the time after the internet comes back online', () => {
+        it('emits ICE failed with 2 seconds delay after XMPP ping comes through', () => {
             iceFailedHandling.start();
-            jasmine.clock().tick(3000);
 
-            networkInfo.updateNetworkInfo({ isOnline: false });
-            jasmine.clock().tick(16000);
-            expect(emitEventSpy).not.toHaveBeenCalled();
+            return nextTick() // tick for ping
+                .then(() => {
+                    expect(emitEventSpy).not.toHaveBeenCalled();
 
-            networkInfo.updateNetworkInfo({ isOnline: true });
-            jasmine.clock().tick(16000);
-            expect(emitEventSpy).toHaveBeenCalled();
+                    return nextTick(2500); // tick for the 2 sec ice timeout
+                })
+                .then(() => {
+                    expect(emitEventSpy).toHaveBeenCalled();
+                });
         });
         it('cancel method cancels the ICE failed event', () => {
             iceFailedHandling.start();
-            jasmine.clock().tick(10000);
-            expect(emitEventSpy).not.toHaveBeenCalled();
 
-            iceFailedHandling.cancel();
-            jasmine.clock().tick(5100);
-            expect(emitEventSpy).not.toHaveBeenCalled();
+            return nextTick(1000) // tick for ping
+                .then(() => {
+                    expect(emitEventSpy).not.toHaveBeenCalled();
+                    iceFailedHandling.cancel();
+
+                    return nextTick(2500); // tick for ice timeout
+                })
+                .then(() => {
+                    expect(emitEventSpy).not.toHaveBeenCalled();
+                });
         });
     });
     describe('when ICE restart are enabled', () => {
@@ -81,9 +80,6 @@ describe('IceFailedHandling', () => {
 
         beforeEach(() => {
             mockConference.options.config.enableIceRestart = true;
-            mockConference.xmpp = {
-                ping: () => Promise.resolve()
-            };
             mockConference.jvbJingleSession = {
                 getIceConnectionState: () => 'failed',
                 // eslint-disable-next-line no-empty-function
@@ -94,23 +90,26 @@ describe('IceFailedHandling', () => {
         it('send ICE failed notification to Jicofo', () => {
             iceFailedHandling.start();
 
-            // first it send ping which is async - need next tick
-            return nextTick().then(() => {
-                jasmine.clock().tick(3000);
-                expect(sendIceFailedSpy).toHaveBeenCalled();
-            });
+            return nextTick() // tick for ping
+                .then(() => nextTick(2500)) // tick for ice timeout
+                .then(() => {
+                    expect(sendIceFailedSpy).toHaveBeenCalled();
+                });
         });
         it('not send ICE failed notification to Jicofo if canceled', () => {
             iceFailedHandling.start();
 
             // first it send ping which is async - need next tick
-            return nextTick().then(() => {
-                jasmine.clock().tick(1000);
-                expect(sendIceFailedSpy).not.toHaveBeenCalled();
-                iceFailedHandling.cancel();
-                jasmine.clock().tick(3000);
-                expect(sendIceFailedSpy).not.toHaveBeenCalled();
-            });
+            return nextTick(1000)
+                .then(() => {
+                    expect(sendIceFailedSpy).not.toHaveBeenCalled();
+                    iceFailedHandling.cancel();
+
+                    return nextTick(3000); // tick for ice timeout
+                })
+                .then(() => {
+                    expect(sendIceFailedSpy).not.toHaveBeenCalled();
+                });
         });
     });
 });

modules/util/TestUtils.js

@@ -1,9 +1,12 @@
 /* global process */
 /**
- * Returns a Promise resolved after {@code process.nextTick}.
+ * Returns a Promise resolved after {@code process.nextTick} with the option to advance Jasmine timers.
  *
+ * @param {number} [advanceTimer] - the value to be passed to Jasmine clock's tick method.
  * @returns {Promise<void>}
  */
-export function nextTick() {
+export function nextTick(advanceTimer) {
+    advanceTimer && jasmine.clock().tick(advanceTimer);
+
     return new Promise(resolve => process.nextTick(resolve));
 }