diff --git a/lib/client/doc.js b/lib/client/doc.js index 2472839e7..129c3be8e 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,10 +1010,16 @@ Doc.prototype._rollback = function(err) { return this._hardRollback(error); } - this._clearInflightOp(err); + // 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) { + this._clearInflightOp(err); + } }; Doc.prototype._hardRollback = function(err) { + console.log('Hard rollcback'); // 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 @@ -1035,14 +1037,24 @@ 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 doc and throw doc error. + logger.error('Hard rollback doc fetch failed.', fetchError, doc); + doc.emit('error', fetchError); + } + var allOpsHadCallbacks = !!pendingOps.length; + console.log('allOpsHadCallbacks', allOpsHadCallbacks); for (var i = 0; i < pendingOps.length; i++) { + console.log('i', i, pendingOps[i].callbacks, err); allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks; } if (err && !allOpsHadCallbacks) return doc.emit('error', err); 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..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}); }); });