Skip to content

Commit

Permalink
🔊 Add log when hard rollback fetch failes
Browse files Browse the repository at this point in the history 
There is a possibility that during the hard rollback the fetch operation
fails, however at the moment we do not do anything with this error. We
do not even log it. This change at least logs the error to notify the client that something went horribly wrong.
  • Loading branch information
@Dawidpol
Dawidpol committed Sep 26, 2023
1 parent b7d0965 commit 6c78853
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 55 deletions.
75 changes: 39 additions & 36 deletions lib/client/doc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,10 +329,6 @@ Doc.prototype._handleSubscribe = function(error, snapshot) {
Doc.prototype._handleOp = function(err, message) {
if (err) {
if (this.inflightOp) {
// The server has rejected submission of the current operation. If we get
// an "Op submit rejected" error, this was done intentionally
// and we should roll back but not return an error to the user.
if (err.code === ERROR_CODE.ERR_OP_SUBMIT_REJECTED) err = null;
return this._rollback(err);
}
return this.emit('error', err);
Expand Down Expand Up @@ -977,48 +973,49 @@ Doc.prototype._opAcknowledged = function(message) {
this._clearInflightOp();
};

Doc.prototype._rollback = function(err) {
Doc.prototype._rollback = function(error) {
// The server has rejected submission of the current operation. Invert by
// just the inflight op if possible. If not possible to invert, cancel all
// pending ops and fetch the latest from the server to get us back into a
// working state, then call back
var op = this.inflightOp;

if ('op' in op && op.type.invert) {
try {
op.op = op.type.invert(op.op);
} catch (error) {
// If the op doesn't support `.invert()`, we just reload the doc
// instead of trying to locally revert it.
return this._hardRollback(err);
}
if (!('op' in op && op.type.invert)) {
return this._hardRollback(error);
}

// Transform the undo operation by any pending ops.
for (var i = 0; i < this.pendingOps.length; i++) {
var transformErr = transformX(this.pendingOps[i], op);
if (transformErr) return this._hardRollback(transformErr);
}
try {
op.op = op.type.invert(op.op);
} catch (invertOpError) {
logger.info('Cannot invert op', op, invertOpError);
// If the op doesn't support `.invert()`, we just reload the doc
// instead of trying to locally revert it.
return this._hardRollback(error);
}

// ... and apply it locally, reverting the changes.
//
// This operation is applied to look like it comes from a remote source.
// I'm still not 100% sure about this functionality, because its really a
// local op. Basically, the problem is that if the client's op is rejected
// by the server, the editor window should update to reflect the undo.
try {
this._otApply(op, false);
} catch (error) {
return this._hardRollback(error);
}
// Transform the undo operation by any pending ops.
for (var i = 0; i < this.pendingOps.length; i++) {
var transformErr = transformX(this.pendingOps[i], op);
if (transformErr) return this._hardRollback(transformErr);
}

this._clearInflightOp(err);
return;
// ... and apply it locally, reverting the changes.
//
// This operation is applied to look like it comes from a remote source.
// I'm still not 100% sure about this functionality, because its really a
// local op. Basically, the problem is that if the client's op is rejected
// by the server, the editor window should update to reflect the undo.
try {
this._otApply(op, false);
} catch (otApplyError) {
logger.info('Cannot apply op in rollback.', op, otApplyError, 'Original error', error);
return this._hardRollback(otApplyError);
}

this._hardRollback(err);
this._clearInflightOp(error);
};

Doc.prototype._hardRollback = function(err) {
Doc.prototype._hardRollback = function(error) {
// 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
Expand All @@ -1036,17 +1033,23 @@ Doc.prototype._hardRollback = function(err) {

// Fetch the latest version from the server to get us back into a working state
var doc = this;
this._fetch({force: true}, function() {
this._fetch({force: true}, function(fetchError) {
// 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.

if (fetchError) {
logger.error('Hard rollback doc fetch failed.', fetchError, doc);
doc.emit('error', fetchError);
}

var allOpsHadCallbacks = !!pendingOps.length;
for (var i = 0; i < pendingOps.length; i++) {
allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks;
allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, error) && allOpsHadCallbacks;
}
if (err && !allOpsHadCallbacks) return doc.emit('error', err);
if (error && !allOpsHadCallbacks) return doc.emit('error', error);
});
};

Expand Down
16 changes: 14 additions & 2 deletions test/client/presence/doc-presence.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,13 @@ describe('DocPresence', function() {
});
});

doc1.submitOp({index: 5, value: 'ern'}, errorHandler(done));
doc1.submitOp({index: 5, value: 'ern'}, errorHandler(function(error) {
if (!error) {
return done('should throw an error');
}

expect(error.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
}));
}
], done);
});
Expand Down Expand Up @@ -741,7 +747,13 @@ describe('DocPresence', function() {
});
});

doc1.submitOp({index: 5, value: 'ern'}, errorHandler(done));
doc1.submitOp({index: 5, value: 'ern'}, errorHandler(function(error) {
if (!error) {
return done('should throw an error');
}

expect(error.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
}));
}
], done);
});
Expand Down
37 changes: 20 additions & 17 deletions test/client/submit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -919,32 +919,34 @@ module.exports = function() {
], done);
});

it('request.rejectedError() soft rejects a create', function(done) {
it('request.rejectedError() rejects a create', function(done) {
this.backend.use('submit', function(request, next) {
next(request.rejectedError());
});
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
if (!err) {
return done('Should emit error');
}
expect(doc.version).equal(0);
expect(doc.data).equal(undefined);
expect(err.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
done();
});
expect(doc.version).equal(null);
expect(doc.data).eql({age: 3});
});

it('request.rejectedError() soft rejects a create without callback', function(done) {
it('request.rejectedError() rejects a create without callback', function(done) {
this.backend.use('submit', function(request, next) {
next(request.rejectedError());
});
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3});
expect(doc.version).equal(null);
expect(doc.data).eql({age: 3});
doc.whenNothingPending(function() {
expect(doc.version).equal(0);
expect(doc.data).equal(undefined);
doc.on('error', function(err) {
expect(err.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
done();
});
});
Expand Down Expand Up @@ -1024,7 +1026,7 @@ module.exports = function() {
});
});

it('request.rejectedError() soft rejects an op', function(done) {
it('request.rejectedError() rejects an op', function(done) {
this.backend.use('submit', function(request, next) {
if ('op' in request.op) return next(request.rejectedError());
next();
Expand All @@ -1033,32 +1035,33 @@ module.exports = function() {
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.submitOp({p: ['age'], na: 1}, function(err) {
if (err) return done(err);
expect(doc.version).equal(1);
expect(doc.data).eql({age: 3});
if (!err) {
return done('Should throw an error');
}

expect(err.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
done();
});
expect(doc.version).equal(1);
expect(doc.data).eql({age: 4});
});
});

it('request.rejectedError() soft rejects an op without callback', function(done) {
it('request.rejectedError() rejects an op without callback', function(done) {
this.backend.use('submit', function(request, next) {
if ('op' in request.op) return next(request.rejectedError());
next();
});
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.submitOp({p: ['age'], na: 1});
expect(doc.version).equal(1);
expect(doc.data).eql({age: 4});
doc.whenNothingPending(function() {
expect(doc.version).equal(1);
expect(doc.data).eql({age: 3});
doc.on('error', function(err) {
if (!err) return done('Should throw an error');

expect(err.code).to.be.equal('ERR_OP_SUBMIT_REJECTED');
done();
});
doc.submitOp({p: ['age'], na: 1});
});
});

Expand Down

0 comments on commit 6c78853

Please sign in to comment.