From 0518ab7bc694a2ac7ac0a6592025cf065ae6b33e Mon Sep 17 00:00:00 2001 From: Alec Gibson Date: Thu, 19 Jul 2018 15:50:04 +0100 Subject: [PATCH 1/3] Fix flaky maxSubmitRetries test This is a slightly speculative fix for a test that fails intermittently on `sharedb-mongo`. I believe these intermittent failures are due to a race condition in a concurrency test. The test works by attempting to fire two commits off at the same time, and hoping that one of them is committed just before the other, so that a `SubmitRequest.retry` is triggered whilst the `maxSubmitRetries` is set to `0`, resulting in an error that is expected. However, I believe it's possible for these commits to (in some cases) happen sequentially rather than concurrently, and fail to error. This change attempts to force them into this retry condition by: - Catching both ops in the `commit` middleware, _just_ before they're about to be committed (and hit a `retry` if applicable) - Waiting until both ops have reached this state - Triggering the first op's `commit` - Then in the callback of that op, triggering the second op's `commit` - The second op should now find that the first op has beaten it to committing, and trigger a `retry` --- test/client/submit.js | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/test/client/submit.js b/test/client/submit.js index 4e508e66e..e4858bc62 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -509,6 +509,7 @@ describe('client submit', function() { }); it('submits fail above the backend.maxSubmitRetries threshold', function(done) { + var backend = this.backend; this.backend.maxSubmitRetries = 0; var doc = this.backend.connect().get('dogs', 'fido'); var doc2 = this.backend.connect().get('dogs', 'fido'); @@ -516,18 +517,24 @@ describe('client submit', function() { if (err) return done(err); doc2.fetch(function(err) { if (err) return done(err); - var count = 0; - var cb = function(err) { - count++; - if (count === 1) { - if (err) return done(err); - } else { - expect(err).ok(); - done(); + var docCallback; + var doc2Callback; + backend.use('commit', function (request, callback) { + if (request.op.op[0].na === 2) docCallback = callback; + if (request.op.op[0].na === 7) doc2Callback = callback; + + if (docCallback && doc2Callback) { + docCallback(); } - }; - doc.submitOp({p: ['age'], na: 2}, cb); - doc2.submitOp({p: ['age'], na: 7}, cb); + }); + doc.submitOp({p: ['age'], na: 2}, function (error) { + if (error) return done(error); + doc2Callback(); + }); + doc2.submitOp({p: ['age'], na: 7}, function (error) { + expect(error).ok(); + done(); + }); }); }); }); From 032eb988f1680e23e9c96a73cdaf19c7ce980887 Mon Sep 17 00:00:00 2001 From: Alec Gibson Date: Thu, 19 Jul 2018 17:31:18 +0100 Subject: [PATCH 2/3] Work around circular sharedb and sharedb-mingo-memory dependency See [this issue][1]. [1]: https://github.com/share/sharedb/pull/226#issuecomment-406297957 --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 7bd066b20..d3e71313a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,6 @@ node_js: - "8" - "6" - "4" -script: "npm run jshint && npm run test-cover" +script: "ln -s .. node_modules/sharedb; npm run jshint && npm run test-cover" # Send coverage data to Coveralls after_script: "cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js" From e9ff681e3dba94a17364caae276ff78de3647fa5 Mon Sep 17 00:00:00 2001 From: Alec Gibson Date: Fri, 20 Jul 2018 07:36:56 +0100 Subject: [PATCH 3/3] Document race condition test fix --- test/client/submit.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/client/submit.js b/test/client/submit.js index e4858bc62..6edc2eb74 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -519,16 +519,28 @@ describe('client submit', function() { if (err) return done(err); var docCallback; var doc2Callback; + // The submit retry happens just after an op is committed. This hook into the middleware + // catches both ops just before they're about to be committed. This ensures that both ops + // are certainly working on the same snapshot (ie one op hasn't been committed before the + // other fetches the snapshot to apply to). By storing the callbacks, we can then + // manually trigger the callbacks, first calling doc, and when we know that's been committed, + // we then commit doc2. backend.use('commit', function (request, callback) { if (request.op.op[0].na === 2) docCallback = callback; if (request.op.op[0].na === 7) doc2Callback = callback; + // Wait until both ops have been applied to the same snapshot and are about to be committed if (docCallback && doc2Callback) { + // Trigger the first op's commit and then the second one later, which will cause the + // second op to retry docCallback(); } }); doc.submitOp({p: ['age'], na: 2}, function (error) { if (error) return done(error); + // When we know the first op has been committed, we try to commit the second op, which will + // fail because it's working on an out-of-date snapshot. It will retry, but exceed the + // maxSubmitRetries limit of 0 doc2Callback(); }); doc2.submitOp({p: ['age'], na: 7}, function (error) {