Ball categorymanager data race (#3797)

* ball_categorymanager atomic d_ruleSetSequenceNumber * ball_category prevent data races surrounding d_relevantRuleMask * ball_category PR fixes, relocate BSLMF_ASSERT * ball_category PR fixes, relocate BSLMF_ASSERT headers
bloomberg · Jul 6, 2022 · a3b45e6 · a3b45e6
1 parent a38bd15
commit a3b45e6
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 9 deletions.
diff --git a/groups/bal/ball/ball_category.cpp b/groups/bal/ball/ball_category.cpp
@@ -8,6 +8,9 @@ BSLS_IDENT_RCSID(ball_category_cpp,"$Id$ $CSID$")
 
 #include <bslim_printer.h>
 
+#include <bslmf_assert.h>
+#include <bslmf_issame.h>
+
 #include <bsls_assert.h>
 
 #include <bsl_algorithm.h>
@@ -20,6 +23,10 @@ namespace ball {
                             // class Category
                             // --------------
 
+// d_relevantRuleMask is semantically of type 'RuleSet::MaskType', but needs
+// to be atomic.  Assert that the type of 'RuleSet::MaskType' hasn't changed.
+BSLMF_ASSERT((bsl::is_same<RuleSet::MaskType, unsigned int>::value));
+
 // PRIVATE CREATORS
 Category::Category(const char       *categoryName,
                    int               recordLevel,
@@ -38,10 +45,11 @@ Category::Category(const char       *categoryName,
                                            triggerAllLevel))
 , d_categoryName(categoryName, basicAllocator)
 , d_categoryHolder(0)
-, d_relevantRuleMask(0)
+, d_relevantRuleMask()
 , d_ruleThreshold(0)
 {
     BSLS_ASSERT(categoryName);
+    bsls::AtomicOperations::initUint(&d_relevantRuleMask, 0);
 }
 
 // PRIVATE MANIPULATORS

diff --git a/groups/bal/ball/ball_category.h b/groups/bal/ball/ball_category.h
@@ -144,7 +144,7 @@ class Category {
 
     CategoryHolder     *d_categoryHolder;   // linked list of holders of this
                                             // category
-    mutable RuleSet::MaskType
+    mutable bsls::AtomicOperations::AtomicTypes::Uint
                         d_relevantRuleMask; // the mask indicating which rules
                                             // are relevant (i.e., have been
                                             // attached to this category)
@@ -489,7 +489,7 @@ int Category::ruleThreshold() const
 inline
 RuleSet::MaskType Category::relevantRuleMask() const
 {
-    return d_relevantRuleMask;
+    return bsls::AtomicOperations::getUintAcquire(&d_relevantRuleMask);
 }
 
                         // --------------------
@@ -577,22 +577,43 @@ void CategoryManagerImpUtil::setRuleThreshold(Category *category,
 inline
 void CategoryManagerImpUtil::enableRule(Category *category, int ruleIndex)
 {
-    category->d_relevantRuleMask =
-        bdlb::BitUtil::withBitSet(category->d_relevantRuleMask, ruleIndex);
+    unsigned int currentMask =
+         bsls::AtomicOperations::getUintRelaxed(&category->d_relevantRuleMask);
+    unsigned int expectedMask;
+    do {
+        const unsigned int updatedMask = bdlb::BitUtil::withBitSet(currentMask,
+                                                                   ruleIndex);
+        expectedMask                   = currentMask;
+        currentMask = bsls::AtomicOperations::testAndSwapUintAcqRel(
+                                                 &category->d_relevantRuleMask,
+                                                 currentMask,
+                                                 updatedMask);
+    } while (expectedMask != currentMask);
 }
 
 inline
 void CategoryManagerImpUtil::disableRule(Category *category, int ruleIndex)
 {
-    category->d_relevantRuleMask =
-        bdlb::BitUtil::withBitCleared(category->d_relevantRuleMask, ruleIndex);
+    unsigned int currentMask =
+         bsls::AtomicOperations::getUintRelaxed(&category->d_relevantRuleMask);
+    unsigned int expectedMask;
+    do {
+        const unsigned int updatedMask =
+                         bdlb::BitUtil::withBitCleared(currentMask, ruleIndex);
+        expectedMask = currentMask;
+        currentMask = bsls::AtomicOperations::testAndSwapUintAcqRel(
+                                                 &category->d_relevantRuleMask,
+                                                 currentMask,
+                                                 updatedMask);
+    } while (expectedMask != currentMask);
 }
 
 inline
 void CategoryManagerImpUtil::setRelevantRuleMask(Category          *category,
                                                  RuleSet::MaskType  mask)
 {
-    category->d_relevantRuleMask = mask;
+    bsls::AtomicOperations::setUintRelease(&category->d_relevantRuleMask,
+                                           mask);
 }
 
 }  // close package namespace

diff --git a/groups/bal/ball/ball_categorymanager.h b/groups/bal/ball/ball_categorymanager.h
@@ -169,6 +169,7 @@ BSLS_IDENT("$Id: $")
 #include <bslmt_readlockguard.h>
 #include <bslmt_readerwriterlock.h>
 
+#include <bsls_atomic.h>
 #include <bsls_types.h>
 
 #include <bsl_map.h>
@@ -195,7 +196,7 @@ class CategoryManager {
                                                       // indices in
                                                       // 'd_categories'
 
-    volatile bsls::Types::Int64      d_ruleSetSequenceNumber;
+    bsls::AtomicInt64                d_ruleSetSequenceNumber;
                                                       // sequence number that
                                                       // is incremented each
                                                       // time the rule set is