Merge pull request #202 from jitsi/session_accept_timeout

Fix session-accept timeout

JitsiConferenceEventManager.js

@@ -178,10 +178,10 @@ JitsiConferenceEventManager.prototype.setupChatRoomListeners = function () {
                     JitsiConferenceErrors.FOCUS_LEFT);
         });
 
-    chatRoom.addListener(XMPPEvents.ALLOCATE_FOCUS_MAX_RETRIES_ERROR,
-        function () {
-            conference.connection._reload();
-        });
+    var reloadHandler = function () { conference.connection._reload(); };
+    chatRoom.addListener(
+        XMPPEvents.ALLOCATE_FOCUS_MAX_RETRIES_ERROR, reloadHandler);
+    chatRoom.addListener(XMPPEvents.SESSION_ACCEPT_TIMEOUT, reloadHandler);
 
     this.chatRoomForwarder.forward(XMPPEvents.CONNECTION_INTERRUPTED,
         JitsiConferenceEvents.CONNECTION_INTERRUPTED);

modules/xmpp/JingleSessionPC.js

@@ -435,15 +435,34 @@ JingleSessionPC.prototype.sendSessionAccept = function (localSDP,
     // Calling tree() to print something useful
     accept = accept.tree();
     logger.info("Sending session-accept", accept);
-
+    var self = this;
     this.connection.sendIQ(accept,
         success,
-        this.newJingleErrorHandler(accept, failure),
+        this.newJingleErrorHandler(accept, function (error) {
+            failure(error);
+            // 'session-accept' is a critical timeout and we'll have to restart
+            self.room.eventEmitter.emit(XMPPEvents.SESSION_ACCEPT_TIMEOUT);
+        }),
         IQ_TIMEOUT);
     // XXX Videobridge needs WebRTC's answer (ICE ufrag and pwd, DTLS
     // fingerprint and setup) ASAP in order to start the connection
     // establishment.
-    this.connection.flush();
+    //
+    // FIXME Flushing the connection at this point triggers an issue with BOSH
+    // request handling in Prosody on slow connections.
+    //
+    // The problem is that this request will be quite large and it may take time
+    // before it reaches Prosody. In the meantime Strophe may decide to send
+    // the next one. And it was observed that a small request with
+    // 'transport-info' usually follows this one. It does reach Prosody before
+    // the previous one was completely received. 'rid' on the server is
+    // increased and Prosody ignores the request with 'session-accept'. It will
+    // never reach Jicofo and everything in the request table is lost. Removing
+    // the flush does not guarantee it will never happen, but makes it much less
+    // likely('transport-info' is bundled with 'session-accept' and any
+    // immediate requests).
+    //
+    // this.connection.flush();
 };
 
 /**

service/xmpp/XMPPEvents.js

@@ -144,6 +144,17 @@ var XMPPEvents = {
      * Indicates that the local sendrecv streams in local SDP are changed.
      */
     SENDRECV_STREAMS_CHANGED: "xmpp.sendrecv_streams_changed",
+    /**
+     * Event fired when we do not get our 'session-accept' acknowledged by
+     * Jicofo. It most likely means that there is serious problem with our
+     * connection or XMPP server and we should reload the conference.
+     *
+     * We have seen that to happen in BOSH requests race condition when the BOSH
+     * request table containing the 'session-accept' was discarded by Prosody.
+     * Jicofo does send the RESULT immediately without any condition, so missing
+     * packets means that most likely it has never seen our IQ.
+     */
+    SESSION_ACCEPT_TIMEOUT: "xmpp.session_accept_timeout",
     // TODO: only used in a hack, should probably be removed.
     SET_LOCAL_DESCRIPTION_ERROR: 'xmpp.set_local_description_error',