Skip to content

Commit

Permalink
Update hard rollback callback logic
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Alec Gibson committed Nov 22, 2018
1 parent d41e034 commit 84a70af
Showing 1 changed file with 19 additions and 7 deletions.
26 changes: 19 additions & 7 deletions lib/client/doc.js
Expand Up @@ -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);
});
};

Expand Down

0 comments on commit 84a70af

Please sign in to comment.