Skip to content

🐛 Wrong error when submitting ops during hard rollback. #692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 4, 2025
Merged

🐛 Wrong error when submitting ops during hard rollback. #692

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f3869c
🐛 Unhandled rejection when submitting ops during hard rollback.
dawidreedsy Mar 3, 2025
fd67b16
Update to call _isInHArdRollback at the top of fetch
dawidreedsy Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions lib/client/doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ function Doc(connection, collection, id) {
this.pendingFetch = [];
this.pendingSubscribe = [];

this._isInHardRollback = false;

// Whether we think we are subscribed on the server. Synchronously set to
// false on calls to unsubscribe and disconnect. Should never be true when
// this.wantSubscribe is false
Expand Down Expand Up @@ -748,10 +750,18 @@ Doc.prototype._submit = function(op, source, callback) {
// The op contains either op, create, delete, or none of the above (a no-op).
if ('op' in op) {
if (!this.type) {
var err = new ShareDBError(
ERROR_CODE.ERR_DOC_DOES_NOT_EXIST,
'Cannot submit op. Document has not been created. ' + this.collection + '.' + this.id
);
if (this._isInHardRollback) {
var err = new ShareDBError(
ERROR_CODE.ERR_DOC_IN_HARD_ROLLBACK,
'Cannot submit op. Document is performing hard rollback. ' + this.collection + '.' + this.id
);
Comment on lines +753 to +757
Copy link
Collaborator

@alecgibson alecgibson Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just move this after the var err = new ShareDBError(ERR_DOC_DOES_NOT_EXIST) to override this "default" error and then avoid having to duplicate the emit() machinery

} else {
var err = new ShareDBError(
ERROR_CODE.ERR_DOC_DOES_NOT_EXIST,
'Cannot submit op. Document has not been created. ' + this.collection + '.' + this.id
);
}

if (callback) return callback(err);
return this.emit('error', err);
}
Expand Down Expand Up @@ -1030,6 +1040,7 @@ Doc.prototype._rollback = function(err) {
};

Doc.prototype._hardRollback = function(err) {
this._isInHardRollback = true;
// 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 Down Expand Up @@ -1077,7 +1088,10 @@ Doc.prototype._hardRollback = function(err) {
inflightOp = null;
}

if (!pendingOps.length) return;
if (!pendingOps.length) {
doc._isInHardRollback = false;
Copy link
Contributor

@ericyhwang ericyhwang Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid potential future issues with forgetting to reset _isInHardRollback on early returns, we could reset to false at the top of this _fetch callback

return;
}
err = new ShareDBError(
ERROR_CODE.ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED,
'Discarding pending op because of hard rollback during ERR_OP_SUBMIT_REJECTED'
Expand All @@ -1090,6 +1104,7 @@ Doc.prototype._hardRollback = function(err) {
allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks;
}
if (err && !allOpsHadCallbacks) doc.emit('error', err);
doc._isInHardRollback = false;
});
};

Expand Down
1 change: 1 addition & 0 deletions lib/error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ ShareDBError.CODES = {
ERR_DOC_DOES_NOT_EXIST: 'ERR_DOC_DOES_NOT_EXIST',
ERR_DOC_TYPE_NOT_RECOGNIZED: 'ERR_DOC_TYPE_NOT_RECOGNIZED',
ERR_DOC_WAS_DELETED: 'ERR_DOC_WAS_DELETED',
ERR_DOC_IN_HARD_ROLLBACK: 'ERR_DOC_IN_HARD_ROLLBACK',
ERR_INFLIGHT_OP_MISSING: 'ERR_INFLIGHT_OP_MISSING',
ERR_INGESTED_SNAPSHOT_HAS_NO_VERSION: 'ERR_INGESTED_SNAPSHOT_HAS_NO_VERSION',
ERR_MAX_SUBMIT_RETRIES_EXCEEDED: 'ERR_MAX_SUBMIT_RETRIES_EXCEEDED',
Expand Down
19 changes: 19 additions & 0 deletions test/client/doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,25 @@ describe('Doc', function() {
}
], done);
});

it('rejects ops with ERR_DOC_IN_HARD_ROLLBACK error when in hard rollback', function(done) {
var backend = this.backend;
var doc = backend.connect().get('dogs', 'snoopy');

async.series([
doc.create.bind(doc, {color: 'red'}),
function(next) {
doc.on('error', function(error) {
expect(error.code).to.be.equal('TEST_ERROR');
});
doc._hardRollback(new ShareDBError('TEST_ERROR'));
doc.submitOp({p: ['color'], oi: 'blue', od: 'red'}, function(error) {
expect(error.code).to.be.equal('ERR_DOC_IN_HARD_ROLLBACK');
next();
});
}
], done);
});
});

describe('errors on ops that could cause prototype corruption', function() {
Expand Down