From 70031bc2bd6c84c7133f7c6ff68659c87fd4165b Mon Sep 17 00:00:00 2001 From: Dawid Kisielewski Date: Tue, 3 Oct 2023 16:16:24 +0200 Subject: [PATCH] CP --- lib/client/doc.js | 60 +++++++++++++++++++++++----- lib/error.js | 5 +++ test/client/doc.js | 33 +++++++++++++++ test/client/presence/doc-presence.js | 16 +++++++- test/client/submit.js | 2 +- 5 files changed, 103 insertions(+), 13 deletions(-) diff --git a/lib/client/doc.js b/lib/client/doc.js index 2472839e7..c3f642d82 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); @@ -1014,6 +1010,13 @@ Doc.prototype._rollback = function(err) { return this._hardRollback(error); } + // 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) { + return this._clearInflightOp(null); + } + this._clearInflightOp(err); }; @@ -1023,9 +1026,8 @@ Doc.prototype._hardRollback = function(err) { // 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); + var pendingOps = this.pendingOps.slice(); + var inflightOp = this.inflightOp; // Cancel all pending ops and reset if we can't invert this._setType(null); @@ -1035,17 +1037,53 @@ 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) { + // This is critical error as it means that our doc is not in usable state + // anymore, we should throw doc error. + logger.error('Hard rollback doc fetch failed.', fetchError, doc); + doc.emit('error', fetchError); + } + + if (err.code !== ERROR_CODE.ERR_OP_SUBMIT_REJECTED) { + if (inflightOp) pendingOps.push(inflightOp); + var allOpsHadCallbacks = !!pendingOps.length; + for (var i = 0; i < pendingOps.length; i++) { + allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks; + } + if (err && !allOpsHadCallbacks) doc.emit('error', err); + return; + } + + /** + * Handle special case of ERR_OP_SUBMIT_REJECTED + * This ensures that we resolve the main op callback and reject + * all the pending ops. This is hard rollback so all the pending ops will be + * discarded. This will ensure that the user is at least informed about it. + * more info: https://github.com/share/sharedb/pull/626 + */ + if (inflightOp) { + util.callEach(inflightOp.callbacks); + } + + if (!pendingOps.length) return; + + var hardRollbackError = new ShareDBError( + ERROR_CODE.ERR_OP_PENDING_OP_SUBMIT_REJECTED, + 'Pending op are present when doing rollback of invertible operation' + ); + 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, hardRollbackError) && allOpsHadCallbacks; } - if (err && !allOpsHadCallbacks) return doc.emit('error', err); + if (!allOpsHadCallbacks) return doc.emit('error', hardRollbackError); }); }; @@ -1061,3 +1099,5 @@ Doc.prototype._clearInflightOp = function(err) { if (err && !called) return this.emit('error', err); }; + + diff --git a/lib/error.js b/lib/error.js index 86f8b9ea6..ec240bbc8 100644 --- a/lib/error.js +++ b/lib/error.js @@ -38,6 +38,11 @@ ShareDBError.CODES = { ERR_OP_ALREADY_SUBMITTED: 'ERR_OP_ALREADY_SUBMITTED', ERR_OP_NOT_ALLOWED_IN_PROJECTION: 'ERR_OP_NOT_ALLOWED_IN_PROJECTION', ERR_OP_SUBMIT_REJECTED: 'ERR_OP_SUBMIT_REJECTED', + /** + * This may happen if server rejected op with ERR_OP_SUBMIT_REJECTED and the type is + * not invertible or there are some pending ops after the create op was rejected with ERR_OP_SUBMIT_REJECTED + */ + ERR_OP_PENDING_OP_SUBMIT_REJECTED: 'ERR_OP_PENDING_OP_SUBMIT_REJECTED', ERR_OP_VERSION_MISMATCH_AFTER_TRANSFORM: 'ERR_OP_VERSION_MISMATCH_AFTER_TRANSFORM', ERR_OP_VERSION_MISMATCH_DURING_TRANSFORM: 'ERR_OP_VERSION_MISMATCH_DURING_TRANSFORM', ERR_OP_VERSION_NEWER_THAN_CURRENT_SNAPSHOT: 'ERR_OP_VERSION_NEWER_THAN_CURRENT_SNAPSHOT', diff --git a/test/client/doc.js b/test/client/doc.js index 671e71f6a..36ff9796d 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -6,6 +6,7 @@ var richText = require('rich-text').type; var ShareDBError = require('../../lib/error'); var errorHandler = require('../util').errorHandler; var sinon = require('sinon'); +var types = require('../../lib/types'); describe('Doc', function() { beforeEach(function() { @@ -514,6 +515,38 @@ describe('Doc', function() { ], done); }); + it('throws an error when hard rollback fetch failed', function(done) { + var backend = this.backend; + doc = this.connection.get('dogs', 'scrappy'); + types.register(richText); + + async.series([ + doc.create.bind(doc, {ops: [{insert: 'Scrappy'}]}, 'rich-text'), + function(next) { + backend.use('reply', function(replyContext, cb) { + if (replyContext.request.a !== 'f') return cb(); + cb({code: 'FETCH_ERROR'}); + }); + backend.use('submit', function(_context, cb) { + cb(new ShareDBError('SUBMIT_ERROR')); + }); + var nonInvertibleOp = [{insert: 'e'}]; + + var count = 0; + function expectError(code) { + count++; + return function(error) { + expect(error.code).to.equal(code); + count--; + if (!count) next(); + }; + } + + doc.on('error', expectError('FETCH_ERROR')); + doc.submitOp(nonInvertibleOp, expectError('SUBMIT_ERROR')); + } + ], done); + }); it('rescues an irreversible op collision', function(done) { // This test case attempts to reconstruct the following corner case, with 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..d8ccf8864 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -934,7 +934,7 @@ module.exports = function() { expect(doc.data).eql({age: 3}); }); - it('request.rejectedError() soft rejects a create without callback', function(done) { + it.only('request.rejectedError() soft rejects a create without callback', function(done) { this.backend.use('submit', function(request, next) { next(request.rejectedError()); });