sims: fix data race in test

include/libsemigroups/runner.hpp

@@ -23,13 +23,13 @@
 #ifndef LIBSEMIGROUPS_RUNNER_HPP_
 #define LIBSEMIGROUPS_RUNNER_HPP_
 
-#include <atomic>   // for atomic
-#include <chrono>   // for nanoseconds, high_resolution_clock
-#include <numeric>  // for accumulate
-#include <thread>   // for ??
+// TODO(0) check thread safety
 
-#include "debug.hpp"      // for LIBSEMIGROUPS_ASSERT
-#include "exception.hpp"  // for LIBSEMIGROUPS_EXCEPTION
+#include <atomic>  // for atomic
+#include <chrono>  // for nanoseconds, high_resolution_clock
+#include <thread>  // for ??
+
+#include "debug.hpp"  // for LIBSEMIGROUPS_ASSERT
 
 #include "detail/function-ref.hpp"  // for FunctionRef
 #include "detail/report.hpp"        // for LibsemigroupsException
@@ -50,8 +50,8 @@ namespace libsemigroups {
     std::string _prefix;
     nanoseconds _report_time_interval;
 
-    mutable time_point _last_report;
-    mutable time_point _start_time;
+    mutable std::atomic<time_point> _last_report;
+    mutable time_point              _start_time;
 
    public:
     // not noexcept because std::string constructor isn't
@@ -66,11 +66,37 @@ namespace libsemigroups {
     }
 
     Reporter& init();
+    // TODO(0) to cpp
+    Reporter(Reporter const& that)
+        : _prefix(that._prefix),
+          _report_time_interval(that._report_time_interval),
+          _last_report(that._last_report.load()),
+          _start_time(that._start_time) {}
+
+    // TODO(0) to cpp
+    Reporter(Reporter&& that)
+        : _prefix(std::move(that._prefix)),
+          _report_time_interval(std::move(that._report_time_interval)),
+          _last_report(that._last_report.load()),
+          _start_time(std::move(that._start_time)) {}
+
+    // TODO(0) to cpp
+    Reporter& operator=(Reporter const& that) {
+      _prefix               = that._prefix;
+      _report_time_interval = that._report_time_interval;
+      _last_report          = that._last_report.load();
+      _start_time           = that._start_time;
+      return *this;
+    }
 
-    Reporter(Reporter const&)            = default;
-    Reporter(Reporter&&)                 = default;
-    Reporter& operator=(Reporter const&) = default;
-    Reporter& operator=(Reporter&&)      = default;
+    // TODO(0) to cpp
+    Reporter& operator=(Reporter&& that) {
+      _prefix               = std::move(that._prefix);
+      _report_time_interval = std::move(that._report_time_interval);
+      _last_report          = that._last_report.load();
+      _start_time           = std::move(that._start_time);
+      return *this;
+    }
 
     ~Reporter() = default;
 
@@ -91,7 +117,7 @@ namespace libsemigroups {
     // TODO(v3) remove!
     [[nodiscard]] inline bool report() const {
       auto t       = std::chrono::high_resolution_clock::now();
-      auto elapsed = t - _last_report;
+      auto elapsed = t - _last_report.load();
 
       if (elapsed > _report_time_interval) {
         _last_report = t;

include/libsemigroups/sims.hpp

@@ -43,6 +43,7 @@
 #ifndef LIBSEMIGROUPS_SIMS_HPP_
 #define LIBSEMIGROUPS_SIMS_HPP_
 
+#include <atomic>
 #include <iostream>
 
 #include <algorithm>   // for max
@@ -85,9 +86,10 @@ namespace libsemigroups {
   //! \sa \ref Sims1
   class SimsStats {
    public:
+    // TODO(0) might be better to have a mutex here and just lock it in
+    // check_point below
     // TODO(doc)
-    // Not atomic because this is only accessed by report_progress_from_thread
-    uint64_t count_last;
+    std::atomic_uint64_t count_last;
     // TODO(doc)
     // Atomic so as to avoid races between report_progress_from_thread and the
     // threads modifying count_last
@@ -113,8 +115,7 @@ namespace libsemigroups {
     //! occur during the running of the algorithms in Sims1. This is the same
     //! as the number of nodes in the search tree encounter during the running
     //! of Sims1.
-    // Not atomic because this is only accessed by report_progress_from_thread
-    uint64_t total_pending_last;
+    std::atomic_uint64_t total_pending_last;
 
     // TODO(doc)
     // Atomic so as to avoid races between report_progress_from_thread and the
@@ -142,8 +143,8 @@ namespace libsemigroups {
     SimsStats& stats_zero();
 
     SimsStats& stats_check_point() {
-      count_last         = count_now;
-      total_pending_last = total_pending_now;
+      count_last         = count_now.load();
+      total_pending_last = total_pending_now.load();
       return *this;
     }
 

src/sims.cpp

@@ -77,10 +77,10 @@ namespace libsemigroups {
   }
 
   SimsStats& SimsStats::init_from(SimsStats const& that) {
-    count_last         = that.count_last;
+    count_last         = that.count_last.load();
     count_now          = that.count_now.load();
     max_pending        = that.max_pending.load();
-    total_pending_last = that.total_pending_last;
+    total_pending_last = that.total_pending_last.load();
     total_pending_now  = that.total_pending_now.load();
     return *this;
   }

tests/test-sims.cpp

@@ -230,8 +230,12 @@ namespace libsemigroups {
 
       it = S.cbegin(3);
       REQUIRE(*it == to_word_graph<node_type>(3, {{0, 0}}));
-      S.for_each(5,
-                 [&S](auto const& wg) { check_right_generating_pairs(S, wg); });
+      // Note that Catch's REQUIRE macro is not thread safe, see:
+      // https://github.com/catchorg/Catch2/issues/99
+      // as such we cannot call any function (like check_right_generating_pairs)
+      // that uses REQUIRE in multiple threads.
+      S.number_of_threads(1).for_each(
+          5, [S](auto const& wg) { check_right_generating_pairs(S, wg); });
     }
     // [[[0, 0]],
     // [[1, 2], [1, 1], [3, 2], [3, 3]],
@@ -3341,7 +3345,11 @@ namespace libsemigroups {
     });
     REQUIRE(count == 52);
 
-    S.for_each(
+    // Note that Catch's REQUIRE macro is not thread safe, see:
+    // https://github.com/catchorg/Catch2/issues/99
+    // as such we cannot call any function (like check_right_generating_pairs)
+    // that uses REQUIRE in multiple threads.
+    S.number_of_threads(1).for_each(
         5, [&S](auto const& wg) { check_two_sided_generating_pairs(S, wg); });
     S.for_each(5,
                [&S](auto const& wg) { check_right_generating_pairs(S, wg); });