Skip to content

Commit

Permalink
♻️ Remove usage of db.getCommittedOpVersion()
Browse files Browse the repository at this point in the history
The `DB.getCommitedOpVersion()` function is only used [in one place][1]:
in a corner case that differentiates between:

 - a create op that conflicts with another create op
 - a create op that has been re-submitted because the connection was
   closed before the ack was received

The first of these cases should result in an error, and the second is
non-fatal and the error can be swallowed.

At the moment, this differentiation is made using the special
`DB.getCommittedOpVersion()` function, which - given an op with a
`src` and `seq` combination - will return `null` if the op hasn't been
committed, or its version number if it has.

If the op has a committed version number, we know that the submit is a
duplicate, and we can safely ignore it.

This approach is problematic, because:

 - it [needs a whole op index][2] just to handle this niche corner case
 - the default behaviour [fetches **all** ops][3] unless the driver
   overrides this behaviour

This change proposes that we actually don't need this function
**at all**, and implements an alternative approach to differentiate
between the above cases.

Instead of having to fetch all ops, we just fetch a *single* op, by
version number, which [is already indexed][4] anyway. We then check to
see if this retrieved op is our creation op.

You can't do this in the general case, because an op's version number
may be changed between submit and commit because of conflicts with
remote ops, which cause it to be transformed up, and its version number
incremented.

**However**, the create op is a **special case**, which
[**cannot** be transformed against **any** other op][5], which means
that the version number it's submitted with **must** be the version
number it's committed with.

We can therefore use this special property of the create op to safely
retrieve itself from the data store by version if it has been committed.

[1]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/submit-request.js#L112
[2]: share/sharedb-mongo#94
[3]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/db/index.js#L69
[4]: https://github.com/share/sharedb-mongo/blob/7e88b0a23e968e3672ee151bf6abaf3bfdf62484/index.js#L401
[5]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/test/ot.js#L152-L162
  • Loading branch information
alecgibson committed May 17, 2024
1 parent 7b20313 commit d8ba01c
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 8 deletions.
24 changes: 16 additions & 8 deletions lib/submit-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,24 @@ SubmitRequest.prototype.submit = function(callback) {
// 'Document already exists' error in response and fail to submit this
// op. However, this could also happen in the case that the op was
// already committed and the create op was simply resent. In that
// case, we should return a non-fatal 'Op already submitted' error. We
// must get the past ops and check their src and seq values to
// differentiate.
backend.db.getCommittedOpVersion(collection, id, snapshot, op, null, function(err, version) {
// case, we should return a non-fatal 'Op already submitted' error.
//
// To check this, let's see if our create op has already been committed.
// It can be uniquely identified by src+seq, so we can compare that with the
// database.
//
// We fetch the op by version number, which is okay with creates because:
// - version can only be changed by transforming the op by remote ops
// - a create op cannot be transformed by any other op
// - therefore, a create op's version **never** changes between submit and
// commit, so we can fetch by version number
backend.db.getOps(collection, id, op.v, op.v + 1, null, function(err, ops) {
if (err) return callback(err);
if (version == null) {
callback(request.alreadyCreatedError());
} else {
op.v = version;
var committedOp = ops[0];
if (op.src === committedOp.src && op.seq === committedOp.seq) {
callback(request.alreadySubmittedError());
} else {
callback(request.alreadyCreatedError());
}
});
return;
Expand Down
23 changes: 23 additions & 0 deletions test/client/submit.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,29 @@ module.exports = function() {
});
});

it('does not fail when resubmitting a create op', function(done) {
var backend = this.backend;
var connection = backend.connect();
var submitted = false;
backend.use('submit', function(request, next) {
if (!submitted) {
submitted = true;
connection.close();
backend.connect(connection);
}
next();
});
var count = 0;
backend.use('reply', function(message, next) {
if (!message.request.create) return next();
count++;
next();
if (count === 2) done();
});
var doc = connection.get('dogs', 'fido');
doc.create({age: 3}, errorHandler(done));
});

it('server fetches and transforms by already committed op', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
var doc2 = this.backend.connect().get('dogs', 'fido');
Expand Down

0 comments on commit d8ba01c

Please sign in to comment.