From cde5144c4217837bc8d439a4eec421c37bd79cad Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 19 Nov 2015 09:46:56 +0000 Subject: [PATCH 1/5] Retry updates that have not been acknowledged. If we do not get a reply from the server acknowledging our update after 5 seconds, send it again. If it never got to the server, this is like normal. If the update got to the server, but we never received the ack then we need to rely on ShareJs's duplicate handling. We set the dupIfSource parameter on any retried updates which let ShareJs know that it's a dup if we already have an op with this version number and client id. The doc-updater and real-time services need changes to correctly send another ack only to the submitting client in the case of a duplicate update. --- .../public/coffee/ide/editor/Document.coffee | 12 ++++------- .../coffee/ide/editor/ShareJsDoc.coffee | 21 +++++++++++++------ 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index d525c7f7bf..14d55b05e9 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -150,14 +150,10 @@ define [ wantToBeJoined: @wantToBeJoined update: update - if Math.random() < (@ide.disconnectRate or 0) - console.log "Simulating disconnect" - @ide.connectionManager.disconnect() - return - - if Math.random() < (@ide.ignoreRate or 0) - console.log "Simulating lost update" - return + if window.dropAcks? and Math.random() < window.dropAcks + if !update.op? # Only drop our own acks, not collaborator updates + console.log "Simulating a lost ack", update + return if update?.doc == @doc_id and @doc? @doc.processUpdateFromServer update diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index 15ba95f56f..83f3d68a77 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee @@ -28,8 +28,13 @@ define [ @connection = { send: (update) => @_startInflightOpTimeout(update) + if window.dropUpdates? and Math.random() < window.dropAcks + console.log "Simulating a lost update", update + return @socket.emit "applyOtUpdate", @doc_id, update state: "ok" + # Unlike ShareJs, our connection.id never changes, even when we disconnect/reconnect. + # This gives this client a unique id used for detecting duplicates ops. id: @socket.socket.sessionid } @@ -90,7 +95,6 @@ define [ updateConnectionState: (state) -> @connection.state = state - @connection.id = @socket.socket.sessionid @_doc.autoOpen = false @_doc._connectionStateChanged(state) @@ -103,13 +107,18 @@ define [ attachToAce: (ace) -> @_doc.attach_ace(ace, false, window.maxDocLength) detachFromAce: () -> @_doc.detach_ace?() - INFLIGHT_OP_TIMEOUT: 10000 + INFLIGHT_OP_TIMEOUT: 5000 _startInflightOpTimeout: (update) -> - meta = - v: update.v - op_sent_at: new Date() timer = setTimeout () => - @trigger "op:timeout", update + # Only send the update again if inflightOp is still populated + # This can be cleared when hard reloading the document in which + # case we don't want to keep trying to send it. + if @_doc.inflightOp? + update.dupIfSource = [@connection.id] + @connection.send(update) + # TODO: Trigger op:timeout only when some max retries have been hit + # and we need to do a full reload. + # @trigger "op:timeout", update , @INFLIGHT_OP_TIMEOUT @_doc.inflightCallbacks.push () => clearTimeout timer From bf452bf306ad7e2bd345b4608435d5b7851e5376 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 19 Nov 2015 11:45:32 +0000 Subject: [PATCH 2/5] Keep the connection.id up to date with socket.io client id The meta.source of an update is populated on the server side so we need to keep our local connection id up to date with it. When a duplicate op is submitted we must send all possible client ids that it could have been sent with. --- .../web/public/coffee/ide/editor/Document.coffee | 6 ++++++ .../web/public/coffee/ide/editor/ShareJsDoc.coffee | 14 +++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index 14d55b05e9..c7cd8c5daf 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -150,6 +150,12 @@ define [ wantToBeJoined: @wantToBeJoined update: update + if window.disconnectOnAck? and Math.random() < window.disconnectOnAck + console.log "Disconnecting on ack", update + window._ide.socket.socket.disconnect() + # Pretend we never received the ack + return + if window.dropAcks? and Math.random() < window.dropAcks if !update.op? # Only drop our own acks, not collaborator updates console.log "Simulating a lost ack", update diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index 83f3d68a77..7d5b6106c6 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee @@ -28,13 +28,14 @@ define [ @connection = { send: (update) => @_startInflightOpTimeout(update) + if window.disconnectOnUpdate? and Math.random() < window.disconnectOnUpdate + console.log "Disconnecting on update", update + window._ide.socket.socket.disconnect() if window.dropUpdates? and Math.random() < window.dropAcks console.log "Simulating a lost update", update return @socket.emit "applyOtUpdate", @doc_id, update state: "ok" - # Unlike ShareJs, our connection.id never changes, even when we disconnect/reconnect. - # This gives this client a unique id used for detecting duplicates ops. id: @socket.socket.sessionid } @@ -95,6 +96,7 @@ define [ updateConnectionState: (state) -> @connection.state = state + @connection.id = @socket.socket.sessionid @_doc.autoOpen = false @_doc._connectionStateChanged(state) @@ -114,7 +116,13 @@ define [ # This can be cleared when hard reloading the document in which # case we don't want to keep trying to send it. if @_doc.inflightOp? - update.dupIfSource = [@connection.id] + # When there is a socket.io disconnect, @_doc.inflightSubmittedIds + # is updated with the socket.io client id of the current op in flight + # (meta.source of the op). + # @connection.id is the client id of the current socket.io session. + # So we need both depending on whether the op was submitted before + # one or more disconnects, or if it was submitted during the current session. + update.dupIfSource = [@connection.id, @_doc.inflightSubmittedIds...] @connection.send(update) # TODO: Trigger op:timeout only when some max retries have been hit # and we need to do a full reload. From 99bca31766833cdbb2b692f4bf36851cc3bb59f7 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 19 Nov 2015 12:02:58 +0000 Subject: [PATCH 3/5] Error and hard refresh document after 30 seconds unsaved changes --- .../public/coffee/ide/editor/Document.coffee | 3 +-- .../coffee/ide/editor/ShareJsDoc.coffee | 22 +++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index c7cd8c5daf..1927b791da 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -242,8 +242,7 @@ define [ doc_id: @doc_id op: op @trigger "op:timeout" - ga?('send', 'event', 'error', "op timeout", "Op was now acknowledged - #{@ide.socket.socket.transport.name}" ) - @ide.connectionManager.reconnectImmediately() + @_onError new Error("op timed out"), {op: op} @doc.on "flush", (inflightOp, pendingOp, version) => @ide.pushEvent "flush", doc_id: @doc_id, diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index 7d5b6106c6..d67961bdf8 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee @@ -109,8 +109,9 @@ define [ attachToAce: (ace) -> @_doc.attach_ace(ace, false, window.maxDocLength) detachFromAce: () -> @_doc.detach_ace?() - INFLIGHT_OP_TIMEOUT: 5000 + INFLIGHT_OP_TIMEOUT: 5000 # Retry sending ops after 5 seconds without an ack _startInflightOpTimeout: (update) -> + @_startFatalTimeoutTimer(update) timer = setTimeout () => # Only send the update again if inflightOp is still populated # This can be cleared when hard reloading the document in which @@ -124,13 +125,26 @@ define [ # one or more disconnects, or if it was submitted during the current session. update.dupIfSource = [@connection.id, @_doc.inflightSubmittedIds...] @connection.send(update) - # TODO: Trigger op:timeout only when some max retries have been hit - # and we need to do a full reload. - # @trigger "op:timeout", update , @INFLIGHT_OP_TIMEOUT @_doc.inflightCallbacks.push () => + @_clearFatalTimeoutTimer() clearTimeout timer + FATAL_OP_TIMEOUT: 30000 # 30 seconds + _startFatalTimeoutTimer: (update) -> + # If an op doesn't get acked within FATAL_OP_TIMEOUT, something has + # gone unrecoverably wrong (the op will have been retried multiple times) + return if @_timeoutTimer? + @_timeoutTimer = setTimeout () => + @_clearFatalTimeoutTimer() + @trigger "op:timeout", update + , @FATAL_OP_TIMEOUT + + _clearFatalTimeoutTimer: () -> + return if !@_timeoutTimer? + clearTimeout @_timeoutTimer + @_timeoutTimer = null + _handleError: (error, meta = {}) -> @trigger "error", error, meta From 442ae638de51b116761a291f754d4ae0fee5da42 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 19 Nov 2015 12:04:35 +0000 Subject: [PATCH 4/5] Revert "Report unsaved changes after 60 seconds" This reverts commit 44a7e598f2fdbd91d18cc340de477ca3d33a1338. --- .../controllers/SavingNotificationController.coffee | 8 -------- 1 file changed, 8 deletions(-) diff --git a/services/web/public/coffee/ide/editor/controllers/SavingNotificationController.coffee b/services/web/public/coffee/ide/editor/controllers/SavingNotificationController.coffee index 0bffb60204..f9c68ca05f 100644 --- a/services/web/public/coffee/ide/editor/controllers/SavingNotificationController.coffee +++ b/services/web/public/coffee/ide/editor/controllers/SavingNotificationController.coffee @@ -24,8 +24,6 @@ define [ if oldStatus[doc_id]? newStatus[doc_id] = oldStatus[doc_id] newStatus[doc_id].unsavedSeconds += 1 - if newStatus[doc_id].unsavedSeconds == 60 - reportUnsavedChanges(doc) else newStatus[doc_id] = { unsavedSeconds: 0 @@ -41,12 +39,6 @@ define [ $scope.docSavingStatusCount = newUnsavedCount $scope.$apply() - reportUnsavedChanges = (doc) -> - ide.reportError new Error("unsaved changes"), { - inflightOp: doc?.getInflightOp(), - pendingOp: doc?.getPendingOp() - } - warnAboutUnsavedChanges = () -> if Document.hasUnsavedChanges() return "You have unsaved changes. If you leave now they will not be saved." From dea97bb9dc413408ce2ab578265196e22d2f160e Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 19 Nov 2015 13:15:51 +0000 Subject: [PATCH 5/5] Fix bad connection debugging parameters --- services/web/public/coffee/ide/editor/ShareJsDoc.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index d67961bdf8..61b9ffb6cf 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee @@ -31,7 +31,7 @@ define [ if window.disconnectOnUpdate? and Math.random() < window.disconnectOnUpdate console.log "Disconnecting on update", update window._ide.socket.socket.disconnect() - if window.dropUpdates? and Math.random() < window.dropAcks + if window.dropUpdates? and Math.random() < window.dropUpdates console.log "Simulating a lost update", update return @socket.emit "applyOtUpdate", @doc_id, update