Skip to content

Commit

Permalink
🗃 Add _create metadata to snapshots to avoid getCommittedOpVersion()
Browse files Browse the repository at this point in the history 
Supersedes #657

The `DB.getCommitedOpVersion()` function is only used [in one place][1]:
in a corner case that differentiates between two "blind", versionless
(created without first fetching) create op conflict cases:

 - a versionless create op that conflicts with another create op
 - a versionless 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 in most
cases, and implements an alternative approach to differentiate between
the above cases.

When creating an snapshot, we store the `src`, `seq` and `v` of the op
that was used to create it.

If a conflicting `create` is received, we can then compare directly with
this metadata, without needing to fetch anything extra from the
database.

This should work in the majority of cases, but won't work if the
metadata is missing, which could happen if:

 - the snapshot is "old": it was created before this change
 - the driver doesn't support metadata (eg [Postgres][4])

In the case where metadata is unavailable, we fall back to the existing
method, using `getCommittedOpVersion()`. However, if drivers support
metadata, this should happen sufficiently infrequently that consumers
could potentially remove the `src`/`seq`/`v` index with no noticeable
performance penalty.

[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-postgres/blob/499702acd478645bcc249fa50ba6fc066d257d04/index.js#L140
  • Loading branch information
@alecgibson
alecgibson committed May 22, 2024
1 parent 22e554d commit 9f08977
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 70 deletions.
25 changes: 24 additions & 1 deletion lib/submit-request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ SubmitRequest.prototype.submit = function(callback) {
// 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) {
request._fetchCreateOpVersion(function(error, version) {
if (err) return callback(err);
if (version == null) {
callback(request.alreadyCreatedError());
Expand Down Expand Up @@ -194,6 +194,15 @@ SubmitRequest.prototype.commit = function(callback) {
backend.trigger(backend.MIDDLEWARE_ACTIONS.commit, this.agent, this, function(err) {
if (err) return callback(err);
if (request._fixupOps.length) request.op.m.fixup = request._fixupOps;
if (request.op.create) {
// When we create the snapshot, we store a pointer to the op that created
// it. This allows us to return OP_ALREADY_SUBMITTED errors when appropriate.
request.snapshot.m._create = {
src: request.op.src,
seq: request.op.seq,
v: request.op.v
};
}

// Try committing the operation and snapshot to the database atomically
backend.db.commit(
Expand Down Expand Up @@ -290,6 +299,20 @@ SubmitRequest.prototype._shouldSaveMilestoneSnapshot = function(snapshot) {
return this.saveMilestoneSnapshot;
};

SubmitRequest.prototype._fetchCreateOpVersion = function(callback) {
var create = this.snapshot.m._create;
if (create) {
var version = (create.src === this.op.src && create.seq === this.op.seq) ? create.v : null;
return callback(null, version);
}

// We can only reach here if the snapshot is missing the create metadata.
// This can happen if a client tries to re-create or resubmit a create op to
// a "legacy" snapshot that existed before we started adding the meta (should
// be uncommon) or when using a driver that doesn't support metadata (eg Postgres).
this.backend.db.getCommittedOpVersion(this.collection, this.id, this.snapshot, this.op, null, callback);
};

// Non-fatal client errors:
SubmitRequest.prototype.alreadySubmittedError = function() {
return new ShareDBError(ERROR_CODE.ERR_OP_ALREADY_SUBMITTED, 'Op already submitted');
Expand Down
164 changes: 95 additions & 69 deletions test/client/submit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var deserializedType = require('./deserialized-type');
var numberType = require('./number-type');
var errorHandler = require('../util').errorHandler;
var richText = require('rich-text');
var MemoryDB = require('../../lib/db/memory');
types.register(deserializedType.type);
types.register(deserializedType.type2);
types.register(numberType.type);
Expand Down Expand Up @@ -206,93 +207,118 @@ module.exports = function() {
});
});

it('can create a new doc then fetch', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.fetch(function(err) {
if (err) return done(err);
expect(doc.data).eql({age: 3});
expect(doc.version).eql(1);
done();
});

describe('create', function() {
describe('metadata enabled', function() {
runCreateTests();
});
});

it('calling create on the same doc twice fails', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.create({age: 4}, function(err) {
expect(err).instanceOf(Error);
expect(doc.version).equal(1);
expect(doc.data).eql({age: 3});
done();
describe('no snapshot metadata available', function() {
beforeEach(function() {
var getSnapshot = MemoryDB.prototype.getSnapshot;
sinon.stub(MemoryDB.prototype, 'getSnapshot')
.callsFake(function(collection, id, fields, snapshotOptions, callback) {
// The $submit field requests metadata, so setting to false will
// imitate a driver that doesn't support metadata; or a snapshot that's
// missing it
if (fields) fields.$submit = false;
getSnapshot.call(this, collection, id, fields, snapshotOptions, callback);
});
});

runCreateTests();
});
});

it('trying to create an already created doc without fetching fails and fetches', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
var doc2 = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc2.create({age: 4}, function(err) {
expect(err).instanceOf(Error);
expect(doc2.version).equal(1);
expect(doc2.data).eql({age: 3});
done();
function runCreateTests() {
it('can create a new doc then fetch', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.fetch(function(err) {
if (err) return done(err);
expect(doc.data).eql({age: 3});
expect(doc.version).eql(1);
done();
});
});
});
});
});

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();
});
it('calling create on the same doc twice fails', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc.create({age: 4}, function(err) {
expect(err).instanceOf(Error);
expect(doc.version).equal(1);
expect(doc.data).eql({age: 3});
done();
});
});
});

var doc = connection.get('dogs', 'fido');
doc.create({age: 3}, function(error) {
expect(doc.version).to.equal(1);
done(error);
});
});
it('trying to create an already created doc without fetching fails and fetches', function(done) {
var doc = this.backend.connect().get('dogs', 'fido');
var doc2 = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc2.create({age: 4}, function(err) {
expect(err).instanceOf(Error);
expect(doc2.version).equal(1);
expect(doc2.data).eql({age: 3});
done();
});
});
});

it('does not fail when resubmitting a create op on a doc that was deleted', function(done) {
var backend = this.backend;
var connection1 = backend.connect();
var connection2 = backend.connect();
var doc1 = connection1.get('dogs', 'fido');
var doc2 = connection2.get('dogs', 'fido');

async.series([
doc1.create.bind(doc1, {age: 3}),
doc1.del.bind(doc1),
function(next) {
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;
connection2.close();
backend.connect(connection2);
connection.close();
backend.connect(connection);
}
next();
});

doc2.create({name: 'Fido'}, function(error) {
expect(doc2.version).to.equal(3);
next(error);
var doc = connection.get('dogs', 'fido');
doc.create({age: 3}, function(error) {
expect(doc.version).to.equal(1);
done(error);
});
}
], done);
});

it('does not fail when resubmitting a create op on a doc that was deleted', function(done) {
var backend = this.backend;
var connection1 = backend.connect();
var connection2 = backend.connect();
var doc1 = connection1.get('dogs', 'fido');
var doc2 = connection2.get('dogs', 'fido');

async.series([
doc1.create.bind(doc1, {age: 3}),
doc1.del.bind(doc1),
function(next) {
var submitted = false;
backend.use('submit', function(request, next) {
if (!submitted) {
submitted = true;
connection2.close();
backend.connect(connection2);
}
next();
});

doc2.create({name: 'Fido'}, function(error) {
expect(doc2.version).to.equal(3);
next(error);
});
}
], done);
});
}
});

it('server fetches and transforms by already committed op', function(done) {
Expand Down

0 comments on commit 9f08977

Please sign in to comment.