SERVER-18994 rework applier draining to avoid possible deadlock

3 files changed, 61 insertions, 6 deletions

diff --git a/jstests/replsets/stepdown_while_draining.js b/jstests/replsets/stepdown_while_draining.js
new file mode 100644
index 00000000000..27646241df3
--- /dev/null
+++ b/jstests/replsets/stepdown_while_draining.js
@@ -0,0 +1,52 @@
+// This test ensures that a node that steps down while draining no longer deadlocks in the producer
+// thread (SERVER-18994):
+//
+// 1) Start a 3 node set
+// 2) Activate node 1's failpoint to trigger the previously problematic behavior (SERVER-18994)
+// 3) Reconfig forcing node 1 to be PRIMARY
+// 4) Reconfig forcing node 2 back into SECONDARY
+// 5) Do a write on the new PRIMARY with w: all to ensure node 1 did not deadlock in producer thread
+//
+// NB: The fail point used by this test simply adds a long sleep, which greatly increases the
+//     likelihood of hitting the previously bad behavior. If this test appears to be flaky on
+//     EverGreen, it is entirely possible that behavior is incorrect and that the inherant raciness
+//     of this test is the source of the flakiness.
+
+load("jstests/replsets/rslib.js");
+
+(function() {
+    "use strict";
+    var name = "StepDownWhileDraining";
+    var replTest = new ReplSetTest({name: name, nodes: 3});
+    var nodes = replTest.nodeList();
+    var conns = replTest.startSet();
+    replTest.initiate({"_id": name,
+                       "version": 1,
+                       "members": [
+                           { "_id": 0, "host": nodes[0], priority: 3 },
+                           { "_id": 1, "host": nodes[1], priority: 0 },
+                           { "_id": 2, "host": nodes[2], arbiterOnly: true}]
+                      });
+
+    var primary = replTest.getPrimary();
+    var config = primary.getDB("local").system.replset.findOne();
+    conns[1].getDB("admin").runCommand({configureFailPoint: 'stepDownWhileDrainingFailPoint',
+                                        mode: 'alwaysOn'});
+    config.version++;
+    config.members[0].priority = 0;
+    config.members[1].priority = 3;
+    primary.getDB(name).foo.insert({x:1});
+    reconfig(replTest, config, true);
+    replTest.waitForState(replTest.nodes[1], replTest.PRIMARY, 60 * 1000);
+
+    config = replTest.nodes[1].getDB("local").system.replset.findOne();
+    config.version++;
+    config.members[1].priority = 0;
+    config.members[0].priority = 3;
+    reconfig(replTest, config, true);
+    replTest.waitForState(replTest.nodes[1], replTest.SECONDARY, 60 * 1000);
+
+    var primary = replTest.getPrimary();
+    assert.writeOK(primary.getDB(name).foo.insert({x:1},
+                                                  {writeConcern: {w: 2, wtimeout: 60 * 1000}}));
+}());
diff --git a/src/mongo/db/repl/bgsync.cpp b/src/mongo/db/repl/bgsync.cpp
index 6c8eba2199e..49ba4ac5668 100644
--- a/src/mongo/db/repl/bgsync.cpp
+++ b/src/mongo/db/repl/bgsync.cpp
@@ -63,6 +63,7 @@ namespace {
 } // namespace
 
     MONGO_FP_DECLARE(rsBgSyncProduce);
+    MONGO_FP_DECLARE(stepDownWhileDrainingFailPoint);
 
     BackgroundSync* BackgroundSync::s_instance = 0;
     boost::mutex BackgroundSync::s_mutex;
@@ -214,11 +215,9 @@ namespace {
                 return;
             }
 
-            // Wait until we've applied the ops we have before we choose a sync target
-            while (!_appliedBuffer && !inShutdownStrict()) {
-                _appliedBufferCondition.wait(lock);
-            }
-            if (inShutdownStrict()) {
+            if (_replCoord->isWaitingForApplierToDrain() ||
+                    _replCoord->getMemberState().primary() ||
+                    inShutdownStrict()) {
                 return;
             }
         }
@@ -339,6 +338,11 @@ namespace {
             BSONObj o = _syncSourceReader.nextSafe().getOwned();
             opsReadStats.increment();
 
+ 
+             if (MONGO_FAIL_POINT(stepDownWhileDrainingFailPoint)) {
+                 sleepsecs(20);
+             }
+
             {
                 boost::unique_lock<boost::mutex> lock(_mutex);
                 _appliedBuffer = false;
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index 55b2be1e97e..9e5671743c8 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -1930,7 +1930,6 @@ namespace {
                 info->master = false;
                 info->condVar->notify_all();
             }
-            _isWaitingForDrainToComplete = false;
             _canAcceptNonLocalWrites = false;
             result = kActionCloseAllConnections;
         }