Update hard rollback callback logic

Alec Gibson · Alec Gibson · commit 84a70afdd879 · 2018-11-22T09:37:25.000Z
We previously followed the following logic for callbacks in a hard
rollback:

  - check there's an inflight op, and that it has a callback
  - if this isn't the case, then emit the error

This logic doesn't work very well in the situation where we don't have
an inflight op. This can happen in the following case:

  - I submit an invalid op
  - the op is added to the pending ops queue
  - we attempt to apply the op to the local document before flushing
  - `type.apply` throws, so the op is never flushed from pending to
    inflight
  - we perform a hard rollback
  - we now find we have no inflight op, but we do have a pending op

In this case, since we have no inflight op, we'd emit the error, but
also call the pending op callback, causing both a callback call _and_
an error emission, which is a bit surprising.

This change updates our logic, so that:

  - we check that there are some ops to callback
  - and that all of those ops have callbacks

If there are no callbacks, or if any of the ops don't handle the error,
then we emit, so that we can be sure of never swallowing an error. We
also avoid treating the inflight op any differently to pending ops, so
that we avoid behaviour that's dependent on where in its lifecycle an op
throws.

However, if the client handles all of their op submission errors, then
we never emit.
diff --git a/lib/client/doc.js b/lib/client/doc.js
@@ -887,22 +887,34 @@ Doc.prototype._rollback = function(err) {
 };
 
 Doc.prototype._hardRollback = function(err) {
+  // Store pending ops so that we can notify their callbacks of the error.
+  // We combine the inflight op and the pending ops, because it's possible
+  // to hit a condition where we have no inflight op, but we do have pending
+  // ops. This can happen when an invalid op is submitted, which causes us
+  // to hard rollback before the pending op was flushed.
+  var pendingOps = [];
+  if (this.inflightOp) pendingOps.push(this.inflightOp);
+  pendingOps = pendingOps.concat(this.pendingOps);
+
   // Cancel all pending ops and reset if we can't invert
-  var op = this.inflightOp;
-  var pending = this.pendingOps;
   this._setType(null);
   this.version = null;
   this.inflightOp = null;
   this.pendingOps = [];
 
-  // Fetch the latest from the server to get us back into a working state
+  // Fetch the latest version from the server to get us back into a working state
   var doc = this;
   this.fetch(function() {
-    var called = op && callEach(op.callbacks, err);
-    for (var i = 0; i < pending.length; i++) {
-      called = callEach(pending[i].callbacks, err) || called;
+    // We want to check that no errors are swallowed, so we check that:
+    // - there are callbacks to call, and
+    // - that every single pending op called a callback
+    // If there are no ops queued, or one of them didn't handle the error,
+    // then we emit the error.
+    var allOpsHadCallbacks = !!pendingOps.length;
+    for (var i = 0; i < pendingOps.length; i++) {
+      allOpsHadCallbacks = callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks;
     }
-    if (err && !called) return doc.emit('error', err);
+    if (err && !allOpsHadCallbacks) return doc.emit('error', err);
   });
 };
 
