SERVER-44666 Make FailPoint::pauseWhileSet not increment _timesEntered

5 files changed, 74 insertions, 29 deletions

diff --git a/jstests/fail_point/fail_point.js b/jstests/fail_point/fail_point.js
index cede99acce7..a4d1f0ce52c 100644
--- a/jstests/fail_point/fail_point.js
+++ b/jstests/fail_point/fail_point.js
@@ -3,13 +3,15 @@
 (function() {
 'use strict';
 
+load("jstests/libs/fail_point_util.js");
+
 /**
- * Performs basic checks on the configureFailPoint command. Also check
- * mongo/util/fail_point_test.cpp for unit tests.
+ * Performs basic checks on the configureFailPoint and waitForFailPoint command.
+ * Also check mongo/util/fail_point_test.cpp for unit tests.
  *
  * @param adminDB {DB} the admin database database object
  */
-function runTest(adminDB) {
+function runBasicTest(adminDB) {
     function expectFailPointState(fpState, expectedMode, expectedData) {
         assert.eq(expectedMode, fpState.mode);
 
@@ -95,13 +97,47 @@ function runTest(adminDB) {
                                  40414);
 }
 
+// Test the parameter handling.
 var conn = MongoRunner.runMongod();
-runTest(conn.getDB('admin'));
+runBasicTest(conn.getDB('admin'));
 MongoRunner.stopMongod(conn);
 
-///////////////////////////////////////////////////////////
-// Test mongos
 var st = new ShardingTest({shards: 1});
-runTest(st.s.getDB('admin'));
+runBasicTest(st.s.getDB('admin'));
+
+// Test the functionality of the commands.
+const testDB = st.shard0.getDB("test");
+const testColl = testDB["user"];
+const failPointName = "hangAfterCollectionInserts";
+
+// Turn on the fail point and check that the returned count is 0.
+var configureFailPointRes = assert.commandWorked(testDB.adminCommand({
+    configureFailPoint: failPointName,
+    mode: "alwaysOn",
+    data: {collectionNS: testColl.getFullName()}
+}));
+assert.eq(0, configureFailPointRes.count);
+
+const joinHungWrite = startParallelShell(() => {
+    assert.commandWorked(db.getSiblingDB("test").user.insert({_id: 0}));
+}, st.rs0.getPrimary().port);
+
+// Wait for the fail point to be entered.
+assert.commandWorked(testDB.adminCommand({
+    waitForFailPoint: failPointName,
+    timesEntered: 1,
+    maxTimeMS: kDefaultWaitForFailPointTimeout
+}));
+
+// Wait for a while before turning it off.
+sleep(200);
+configureFailPointRes =
+    assert.commandWorked(testDB.adminCommand({configureFailPoint: failPointName, mode: "off"}));
+
+// Check that waiting does not cause the count to be incremented.
+assert.eq(1, configureFailPointRes.count);
+
+joinHungWrite();
+
 st.stop();
 })();
diff --git a/jstests/noPassthrough/transaction_coordinator_curop_info.js b/jstests/noPassthrough/transaction_coordinator_curop_info.js
index 926230567af..748ea166889 100644
--- a/jstests/noPassthrough/transaction_coordinator_curop_info.js
+++ b/jstests/noPassthrough/transaction_coordinator_curop_info.js
@@ -25,7 +25,7 @@ function commitTxn(st, lsid, txnNumber, expectedError = null) {
 }
 
 function curOpAfterFailpoint(failPoint, filter, timesEntered = 1) {
-    jsTest.log(`waiting for failpoint '${failPoint.failPointName}' to appear in the log ${
+    jsTest.log(`waiting for failpoint '${failPoint.failPointName}' to be entered ${
         timesEntered} time(s).`);
     failPoint.wait(timesEntered);
 
diff --git a/jstests/sharding/refine_collection_shard_key_basic.js b/jstests/sharding/refine_collection_shard_key_basic.js
index 322045a340c..368c6cbf0e5 100644
--- a/jstests/sharding/refine_collection_shard_key_basic.js
+++ b/jstests/sharding/refine_collection_shard_key_basic.js
@@ -340,7 +340,7 @@ const awaitShellToTriggerStaleEpoch = startParallelShell(() => {
         db.adminCommand({refineCollectionShardKey: 'db.foo', key: {_id: 1, aKey: 1}}),
         ErrorCodes.StaleEpoch);
 }, staleMongos.port);
-hangAfterRefreshFailPoint.wait(2);
+hangAfterRefreshFailPoint.wait();
 
 // Drop and re-shard namespace 'db.foo' without staleMongos refreshing its metadata.
 dropAndReshardColl({_id: 1});
diff --git a/src/mongo/util/fail_point.cpp b/src/mongo/util/fail_point.cpp
index c353780e57b..c714c1cacdd 100644
--- a/src/mongo/util/fail_point.cpp
+++ b/src/mongo/util/fail_point.cpp
@@ -131,7 +131,7 @@ void FailPoint::_disable() {
     _fpInfo.fetchAndBitAnd(~kActiveBit);
 }
 
-FailPoint::RetCode FailPoint::_slowShouldFailOpenBlockImpl(
+FailPoint::RetCode FailPoint::_slowShouldFailOpenBlockWithoutIncrementingTimesEntered(
     std::function<bool(const BSONObj&)> cb) noexcept {
     ValType localFpInfo = _fpInfo.addAndFetch(1);
 
@@ -176,7 +176,7 @@ FailPoint::RetCode FailPoint::_slowShouldFailOpenBlockImpl(
 
 FailPoint::RetCode FailPoint::_slowShouldFailOpenBlock(
     std::function<bool(const BSONObj&)> cb) noexcept {
-    auto ret = _slowShouldFailOpenBlockImpl(cb);
+    auto ret = _slowShouldFailOpenBlockWithoutIncrementingTimesEntered(cb);
     if (ret == slowOn) {
         _timesEntered.addAndFetch(1);
     }
diff --git a/src/mongo/util/fail_point.h b/src/mongo/util/fail_point.h
index b14693d0db1..dd8bd8c9916 100644
--- a/src/mongo/util/fail_point.h
+++ b/src/mongo/util/fail_point.h
@@ -107,6 +107,8 @@ class FailPoint {
 private:
     enum RetCode { fastOff = 0, slowOff, slowOn, userIgnored };
 
+    enum ShouldFailEntryMode { kFirstTimeEntered, kEnteredAlready };
+
 public:
     using ValType = unsigned;
     enum Mode { off, alwaysOn, random, nTimes, skip };
@@ -201,17 +203,18 @@ public:
     /**
      * Returns true if fail point is active.
      *
-     * Calls to `shouldFail` can have side effects. For example they affect the counters
-     * kept to manage the `skip` or `nTimes` modes (See `setMode`).
-     *
-     * See `executeIf` for information on `pred`.
+     * @param pred       see `executeIf` for more information.
+     * @param entryMode  kEnteredAlready if the caller has already entered the fail point,
+     *                   and kFirstTimeEntered otherwise. If `entryMode` is kFirstTimeEntered,
+     *                   calls to `shouldFail` can have side effects. For example, they affect
+     *                   the counters kept to manage the `skip` or `nTimes` modes (See `setMode`).
      *
      * Calls to `shouldFail` should be placed inside MONGO_unlikely for performance.
      *    if (MONGO_unlikely(failpoint.shouldFail())) ...
      */
     template <typename Pred>
-    bool shouldFail(Pred&& pred) {
-        RetCode ret = _shouldFailOpenBlock(std::forward<Pred>(pred));
+    bool shouldFail(Pred&& pred, ShouldFailEntryMode entryMode = kFirstTimeEntered) {
+        RetCode ret = _shouldFailOpenBlock(std::forward<Pred>(pred), entryMode);
 
         if (MONGO_likely(ret == fastOff)) {
             return false;
@@ -221,8 +224,8 @@ public:
         return ret == slowOn;
     }
 
-    bool shouldFail() {
-        return shouldFail(nullptr);
+    bool shouldFail(ShouldFailEntryMode entryMode = kFirstTimeEntered) {
+        return shouldFail(nullptr, entryMode);
     }
 
     /**
@@ -290,7 +293,7 @@ public:
      */
     template <typename Pred>
     Scoped scopedIf(Pred&& pred) {
-        return Scoped(this, _shouldFailOpenBlock(std::forward<Pred>(pred)));
+        return Scoped(this, _shouldFailOpenBlock(std::forward<Pred>(pred), kFirstTimeEntered));
     }
 
     template <typename F>
@@ -318,7 +321,7 @@ public:
      * This uses `shouldFail()` and therefore affects FailPoint counters.
      */
     void pauseWhileSet() {
-        while (MONGO_unlikely(shouldFail())) {
+        while (MONGO_unlikely(shouldFail(kEnteredAlready))) {
             sleepmillis(100);
         }
     }
@@ -329,7 +332,7 @@ public:
      * OperationContext).
      */
     void pauseWhileSet(OperationContext* opCtx) {
-        while (MONGO_unlikely(shouldFail())) {
+        while (MONGO_unlikely(shouldFail(kEnteredAlready))) {
             opCtx->sleepFor(Milliseconds(100));
         }
     }
@@ -343,7 +346,8 @@ private:
      * decrementing it. Must call shouldFailCloseBlock afterwards when the return value
      * is not fastOff. Otherwise, this will remain read-only forever.
      *
-     * Note: see `executeIf` for information on `pred`.
+     * Note: see `executeIf` for information on `pred`, and `shouldFail` for information
+     *       on `entryMode`.
      *
      * @return slowOn if its active and needs to be closed
      *         userIgnored if its active and needs to be closed, but shouldn't be acted on
@@ -351,16 +355,20 @@ private:
      *         fastOff if its disabled and doesn't need to be closed
      */
     template <typename Pred>
-    RetCode _shouldFailOpenBlock(Pred&& pred) {
+    RetCode _shouldFailOpenBlock(Pred&& pred, ShouldFailEntryMode entryMode) {
         if (MONGO_likely((_fpInfo.loadRelaxed() & kActiveBit) == 0)) {
             return fastOff;
         }
 
+        if (entryMode == kEnteredAlready) {
+            return _slowShouldFailOpenBlockWithoutIncrementingTimesEntered(
+                std::forward<Pred>(pred));
+        }
         return _slowShouldFailOpenBlock(std::forward<Pred>(pred));
     }
 
-    RetCode _shouldFailOpenBlock() {
-        return _shouldFailOpenBlock(nullptr);
+    RetCode _shouldFailOpenBlock(ShouldFailEntryMode entryMode) {
+        return _shouldFailOpenBlock(nullptr, entryMode);
     }
 
     /**
@@ -375,13 +383,14 @@ private:
      * If a callable is passed, and returns false, this will return userIgnored and avoid altering
      * the mode in any way.  The argument is the fail point payload.
      */
-    RetCode _slowShouldFailOpenBlockImpl(std::function<bool(const BSONObj&)> cb) noexcept;
+    RetCode _slowShouldFailOpenBlockWithoutIncrementingTimesEntered(
+        std::function<bool(const BSONObj&)> cb) noexcept;
 
     /**
      * slow path for #_shouldFailOpenBlock
      *
-     * Calls _slowShouldFailOpenBlockImpl. If it returns slowOn, increments the number of times
-     * the fail point has been entered before returning the RetCode.
+     * Calls _slowShouldFailOpenBlockWithoutIncrementingTimesEntered. If it returns slowOn,
+     * increments the number of times the fail point has been entered before returning the RetCode.
      */
     RetCode _slowShouldFailOpenBlock(std::function<bool(const BSONObj&)> cb) noexcept;