From 6c788533daef23e0bfb7b063adcf4cf97852e898 Mon Sep 17 00:00:00 2001 From: Dawid Kisielewski Date: Tue, 19 Sep 2023 14:42:54 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8A=20Add=20log=20when=20hard=20rollba?= =?UTF-8?q?ck=20fetch=20failes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/client/doc.js | 75 +++++++++++++++------------- test/client/presence/doc-presence.js | 16 +++++- test/client/submit.js | 37 +++++++------- 3 files changed, 73 insertions(+), 55 deletions(-) diff --git a/lib/client/doc.js b/lib/client/doc.js index 7bb47c33f..ad4b7aefc 100644 --- a/lib/client/doc.js +++ b/lib/client/doc.js @@ -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); @@ -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 @@ -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); }); }; diff --git a/test/client/presence/doc-presence.js b/test/client/presence/doc-presence.js index 1011ac74b..ef1fcef48 100644 --- a/test/client/presence/doc-presence.js +++ b/test/client/presence/doc-presence.js @@ -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); }); @@ -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); }); diff --git a/test/client/submit.js b/test/client/submit.js index 37716dc41..3f31f8a21 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -919,22 +919,25 @@ 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()); }); @@ -942,9 +945,8 @@ module.exports = function() { 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(); }); }); @@ -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(); @@ -1033,9 +1035,11 @@ 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); @@ -1043,7 +1047,7 @@ module.exports = function() { }); }); - 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(); @@ -1051,14 +1055,13 @@ module.exports = function() { 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}); }); });