Remove atomic from shmem. Implement mutex for local writer locking

This controls concurrency on the event_detect writer side. The reader
takes the risk of a torn read. Note that an architecural review of
this, there is concurrence due to the low update frequency that
the chances of a torn read are low, and atomics can't be implemented
on C++03 code on some DC systems, and the overhead of using
pthread_mutex_t is not worth it.

Author: jamescowens

event_detect.cpp

@@ -80,20 +80,6 @@ SharedMemoryTimestampExporter g_shmem_exporter(SHMEM_NAME_CONFIG);
 //!
 std::atomic<bool> g_shm_initialized_successfully = false;
 
-// Note this is to check that the data structures are the same between the std::atomic version of in64_t and the
-// raw version, because some DC platforms do not have access to std::atomic on their side, and so will read these
-// raw. Note that the chances of a torn read is very low, and on the write side, we are guaranteeing atomic writes
-// by the use of std::atomic.
-static_assert(sizeof(std::atomic<int64_t>) == sizeof(int64_t),
-              "Size mismatch between atomic<int64_t> and int64_t");
-static_assert(alignof(std::atomic<int64_t>) == alignof(int64_t),
-              "Alignment mismatch between atomic<int64_t> and int64_t");
-// Check the array specifically
-static_assert(sizeof(std::atomic<int64_t>[2]) == sizeof(int64_t[2]),
-              "Size mismatch between atomic<int64_t>[2] and int64_t[2]");
-static_assert(alignof(std::atomic<int64_t>[2]) == alignof(int64_t[2]),
-              "Alignment mismatch between atomic<int64_t>[2] and int64_t[2]");
-
 // Class Monitor
 
 Monitor::Monitor()
@@ -976,6 +962,9 @@ bool SharedMemoryTimestampExporter::CreateOrOpen(mode_t mode) {
     debug_log("INFO: %s: Initializing shared memory segment %s for atomic int64_t[2]", __func__, m_shm_name.c_str());
 
     Cleanup(); // Ensure clean state
+
+    std::unique_lock<std::mutex> lock(mtx_shmem);
+
     m_is_creator = false;
 
     // 1. Create or open RW
@@ -1028,12 +1017,11 @@ bool SharedMemoryTimestampExporter::CreateOrOpen(mode_t mode) {
     }
 
     // 5. Store pointer, initialize if we created/resized it
-    m_mapped_ptr = static_cast<std::atomic<int64_t>*>(mapped_mem);
+    m_mapped_ptr = static_cast<int64_t*>(mapped_mem);
     if (m_is_creator) {
-        int64_t initial_time = GetUnixEpochTime(); // Assumes GetUnixEpochTime() is available
-        // Initialize both elements atomically
-        m_mapped_ptr[0].store(initial_time, std::memory_order_relaxed); // update_time
-        m_mapped_ptr[1].store(initial_time, std::memory_order_relaxed); // last_active_time
+        int64_t initial_time = GetUnixEpochTime();
+        m_mapped_ptr[0] = initial_time; // update_time
+        m_mapped_ptr[1] = initial_time; // last_active_time
         debug_log("INFO: %s: Shared memory segment %s initialized. update=%lld, last_active=%lld",
                   __func__, m_shm_name.c_str(), (long long)initial_time, (long long)initial_time);
     } else {
@@ -1045,18 +1033,16 @@ bool SharedMemoryTimestampExporter::CreateOrOpen(mode_t mode) {
 }
 
 bool SharedMemoryTimestampExporter::UpdateTimestamps(int64_t update_time, int64_t last_active_time) {
+    std::unique_lock<std::mutex> lock(mtx_shmem);
+
     if (!m_is_initialized.load() || m_mapped_ptr == nullptr) {
-        // Log periodically? Avoid flooding logs.
-        // static std::atomic<int> s_log_counter = 0;
-        // if (s_log_counter++ % 60 == 0) // Log once a minute maybe
-        //    error_log("ERROR: %s: Cannot update timestamp, shared memory not initialized.", __func__);
         return false;
     }
     // Use relaxed memory order: assumes readers don't need strict ordering
     // relative to other non-atomic operations in this thread. Atomicity
     // of the store itself is guaranteed.
-    m_mapped_ptr[0].store(update_time, std::memory_order_relaxed);      // Store update_time at index 0
-    m_mapped_ptr[1].store(last_active_time, std::memory_order_relaxed); // Store last_active_time at index 1
+    m_mapped_ptr[0] = update_time;      // Store update_time at index 0
+    m_mapped_ptr[1] = last_active_time; // Store last_active_time at index 1
     return true;
 }
 
@@ -1065,6 +1051,8 @@ bool SharedMemoryTimestampExporter::IsInitialized() const {
 }
 
 void SharedMemoryTimestampExporter::Cleanup() { // Renamed from Close
+    std::unique_lock<std::mutex> lock(mtx_shmem);
+
     if (m_mapped_ptr != nullptr) {
         debug_log("DEBUG: %s: Unmapping shared memory %s...", __func__, m_shm_name.c_str());
         if (munmap(m_mapped_ptr, m_size) == -1) { // <-- Use correct size
@@ -1084,6 +1072,8 @@ void SharedMemoryTimestampExporter::Cleanup() { // Renamed from Close
 }
 
 bool SharedMemoryTimestampExporter::UnlinkSegment() {
+    std::unique_lock<std::mutex> lock(mtx_shmem);
+
     debug_log("INFO: %s: Requesting unlink for shared memory %s...", __func__, m_shm_name.c_str());
     errno = 0;
     if (shm_unlink(m_shm_name.c_str()) == -1) {

event_detect.h

@@ -411,7 +411,7 @@ class IdleDetectMonitor
 
 /**
  * @brief Manages a POSIX shared memory segment for exporting timestamps.
- * Stores an array of two atomic int64_t: {update_time, last_active_time}.
+ * Stores an array of two int64_t: {update_time, last_active_time}.
  * Handles creation, mapping, updating, and cleanup via RAII.
  */
 class SharedMemoryTimestampExporter {
@@ -435,15 +435,15 @@ class SharedMemoryTimestampExporter {
 
     /**
      * @brief Creates (if necessary) and opens the shared memory segment,
-     * sets its size (to sizeof(atomic<int64_t>[2])), and maps it for writing.
+     * sets its size (to sizeof(int64_t[2])), and maps it for writing.
      * Must be called before UpdateTimestamps or IsInitialized.
      * @param mode Permissions (e.g., 0666 or 0660) to use if creating the segment.
      * @return True on success, false on any failure (shm_open, ftruncate, mmap).
      */
     bool CreateOrOpen(mode_t mode = 0666);
 
     /**
-     * @brief Atomically updates both timestamps in the mapped shared memory.
+     * @brief Updates both timestamps in the mapped shared memory.
      * @param update_time The timestamp of the current update cycle.
      * @param last_active_time The calculated overall last active time.
      * @return True if updated successfully, false if not initialized or pointer is invalid.
@@ -470,10 +470,19 @@ class SharedMemoryTimestampExporter {
      */
     void Cleanup();
 
+    //!
+    //! \brief This protects against multiple threads in the event_detect process from simultaneously accessing
+    //! and writing to the shared memory segment. It does NOT protect from another process encountering a torn
+    //! read. I would prefer to use a pthread_mutex_t in the shmem data structure and manege the mutex across
+    //! the writer and reader, but the BOINC architect does not believe this is necessary in practice given the small
+    //! size and low frequency of writing and reading.
+    //!
+    std::mutex mtx_shmem;
+
     std::string m_shm_name;
     int m_shm_fd;
-    std::atomic<int64_t>* m_mapped_ptr; // Pointer to the start of the atomic int64_t[2] array
-    const size_t m_size;               // Size of the atomic int64_t[2] array
+    int64_t* m_mapped_ptr;             // Pointer to the start of the int64_t[2] array
+    const size_t m_size;               // Size of the int64_t[2] array
     bool m_is_creator;                 // Did this instance create/resize the segment?
     std::atomic<bool> m_is_initialized;
 };

read_shmem_timestamps.cpp

@@ -5,7 +5,6 @@
  */
 
 #include <string>
-#include <atomic>
 #include <cstdint>
 #include <cstring>   // For strerror
 #include <iostream>  // Keep for final std::cout output
@@ -22,7 +21,7 @@
 
 // Define constants for the shared memory layout
 const char* DEFAULT_SHMEM_NAME = "/idle_detect_shmem";
-const size_t SHMEM_SIZE = sizeof(std::atomic<int64_t>[2]); // Size of the array
+const size_t SHMEM_SIZE = sizeof(int64_t[2]); // Size of the array
 
 int main(int argc, char* argv[]) {
     // --- Argument Parsing ---
@@ -53,7 +52,7 @@ int main(int argc, char* argv[]) {
     // --- Read Shared Memory ---
     int shm_fd = -1;
     void* mapped_mem = MAP_FAILED;
-    std::atomic<int64_t>* shm_atomic_ptr = nullptr;
+    int64_t* shm_ptr = nullptr;
     int64_t update_time = -1;
     int64_t last_active_time = -1;
     bool read_success = false;
@@ -77,21 +76,20 @@ int main(int argc, char* argv[]) {
     }
 
     // --- Access Data ---
-    shm_atomic_ptr = static_cast<std::atomic<int64_t>*>(mapped_mem);
-    // Atomically load both values from the array
-    update_time = shm_atomic_ptr[0].load(std::memory_order_relaxed);
-    last_active_time = shm_atomic_ptr[1].load(std::memory_order_relaxed);
+    shm_ptr = static_cast<int64_t*>(mapped_mem);
+    update_time = shm_ptr[0];
+    last_active_time = shm_ptr[1];
     read_success = true;
 
     // Unmap memory
     errno = 0;
-    if (munmap(mapped_mem, sizeof(std::atomic<int64_t>)) == -1) {
+    if (munmap(mapped_mem, SHMEM_SIZE) == -1) {
         log("WARN: %s: munmap failed for shm '%s': %s (%d)",
             __func__, shm_name, strerror(errno), errno);
         // Continue as we already have the value
     }
     mapped_mem = nullptr;
-    shm_atomic_ptr = nullptr;
+    shm_ptr = nullptr;
 
     // --- Unmap ---
     errno = 0;