Code review

.github/workflows/build.yml

@@ -245,6 +245,7 @@ jobs:
       shell: bash
       run: |
         export CTEST_OUTPUT_ON_FAILURE=TRUE
+        export MACOSX_DEPLOYMENT_TARGET=15.0
         conan create . --build=missing -pr conan/profiles/macos  -o '&:shared=${{ matrix.shared }}' -o '&:with_docs=False'
 
     - name: Clean conan cache

CMakeLists.txt

@@ -2,7 +2,7 @@
 set(AGENT_VERSION_MAJOR 2)
 set(AGENT_VERSION_MINOR 7)
 set(AGENT_VERSION_PATCH 0)
-set(AGENT_VERSION_BUILD 7)
+set(AGENT_VERSION_BUILD 8)
 set(AGENT_VERSION_RC "")
 
 # This minimum version is to support Visual Studio 2019 and C++ feature checking and FetchContent

agent_lib/CMakeLists.txt

@@ -171,9 +171,11 @@ set(AGENT_SOURCES
 # src/parser HEADER_FILE_ONLY
 
         "${SOURCE_DIR}/parser/xml_parser.hpp"
+        "${SOURCE_DIR}/parser/json_parser.hpp"
 
 # src/parser SOURCE_FILES_ONLY
 
+        "${SOURCE_DIR}/parser/json_parser.cpp"
         "${SOURCE_DIR}/parser/xml_parser.cpp"
 
 # src/pipeline HEADER_FILE_ONLY
@@ -328,7 +330,6 @@ find_package(Boost REQUIRED)
 find_package(LibXml2 REQUIRED)
 find_package(date REQUIRED)
 find_package(OpenSSL REQUIRED)
-find_package(nlohmann_json REQUIRED)
 find_package(mqtt_cpp REQUIRED)
 find_package(RapidJSON REQUIRED)
 
@@ -403,7 +404,7 @@ target_link_libraries(
   agent_lib
   PUBLIC
   boost::boost LibXml2::LibXml2 date::date openssl::openssl
-  nlohmann_json::nlohmann_json mqtt_cpp::mqtt_cpp 
+  mqtt_cpp::mqtt_cpp 
   rapidjson BZip2::BZip2
 
   $<$<PLATFORM_ID:Linux>:pthread>

src/mtconnect/agent.cpp

@@ -325,7 +325,6 @@ namespace mtconnect {
       }
     }
 
-    std::lock_guard<buffer::CircularBuffer> lock(m_circularBuffer);
     if (m_circularBuffer.addToBuffer(observation) != 0)
     {
       for (auto &sink : m_sinks)

src/mtconnect/buffer/checkpoint.cpp

@@ -154,7 +154,7 @@ namespace mtconnect {
       }
       else
       {
-        m_observations[id] = dynamic_pointer_cast<Observation>(obs->getptr());
+        m_observations[id] = obs;
       }
     }
 
@@ -170,7 +170,7 @@ namespace mtconnect {
       for (const auto &event : checkpoint.m_observations)
       {
         if (!m_filter || m_filter->count(event.first) > 0)
-          m_observations[event.first] = dynamic_pointer_cast<Observation>(event.second->getptr());
+          m_observations[event.first] = event.second;
       }
     }
 

src/mtconnect/buffer/circular_buffer.hpp

@@ -126,36 +126,43 @@ namespace mtconnect::buffer {
       if (observation->isOrphan())
         return 0;
 
-      std::lock_guard<std::recursive_mutex> lock(m_sequenceLock);
-      auto dataItem = observation->getDataItem();
-      auto seq = m_sequence;
-
-      observation->setSequence(seq);
-      m_slidingBuffer.push_back(observation);
-      m_latest.addObservation(observation);
+      SequenceNumber_t seq;
+      DataItemPtr dataItem;
 
-      // Special case for the first event in the series to prime the first checkpoint.
-      if (seq == 1)
-        m_first.addObservation(observation);
-      else if (m_slidingBuffer.full())
       {
-        observation::ObservationPtr old = m_slidingBuffer.front();
-        m_first.addObservation(old);
-        if (old->getSequence() > 1)
-          m_firstSequence++;
-        // assert(old->getSequence() == m_firstSequence);
-      }
+        std::lock_guard<std::recursive_mutex> lock(m_sequenceLock);
+        dataItem = observation->getDataItem();
+        seq = m_sequence;
+
+        observation->setSequence(seq);
+        m_slidingBuffer.push_back(observation);
+        m_latest.addObservation(observation);
+
+        // Special case for the first event in the series to prime the first checkpoint.
+        if (seq == 1)
+          m_first.addObservation(observation);
+        else if (m_slidingBuffer.full())
+        {
+          observation::ObservationPtr old = m_slidingBuffer.front();
+          m_first.addObservation(old);
+          if (old->getSequence() > 1)
+            m_firstSequence++;
+          // assert(old->getSequence() == m_firstSequence);
+        }
 
-      // Checkpoint management
-      if (m_checkpointCount > 0 && (seq % m_checkpointFreq) == 0)
-      {
-        // Copy the checkpoint from the current into the slot
-        m_checkpoints.push_back(std::make_unique<Checkpoint>(m_latest));
-      }
+        // Checkpoint management
+        if (m_checkpointCount > 0 && (seq % m_checkpointFreq) == 0)
+        {
+          // Copy the checkpoint from the current into the slot
+          m_checkpoints.push_back(std::make_unique<Checkpoint>(m_latest));
+        }
 
-      dataItem->signalObservers(m_sequence);
+        m_sequence++;
+      }
 
-      m_sequence++;
+      // Signal observers outside the lock to avoid three-deep lock nesting
+      // (m_sequenceLock -> m_observerMutex -> observer m_mutex) on every write
+      dataItem->signalObservers(seq);
 
       return seq;
     }

src/mtconnect/configuration/agent_config.cpp

@@ -796,12 +796,12 @@ namespace mtconnect::configuration {
     catch (boost::property_tree::json_parser::json_parser_error &e)
     {
       cerr << "json file error: " << e.what() << " on line " << e.line() << endl;
-      throw e;
+      throw;
     }
-    catch (std::exception e)
+    catch (const std::exception &e)
     {
       cerr << "could not load config file: " << e.what() << endl;
-      throw e;
+      throw;
     }
 
     if (m_logChannels.empty())

src/mtconnect/entity/entity.hpp

@@ -44,12 +44,12 @@ namespace mtconnect {
       PropertyKey(const char *s) : QName(s) {}
 
       /// @brief clears marks for this property
-      void clearMark() const { const_cast<PropertyKey *>(this)->m_mark = false; }
+      void clearMark() const { m_mark = false; }
       /// @brief sets the mark for this property
-      void setMark() const { const_cast<PropertyKey *>(this)->m_mark = true; }
+      void setMark() const { m_mark = true; }
 
       /// @brief allows factory to track required properties
-      bool m_mark {false};
+      mutable bool m_mark {false};
     };
 
     /// @brief properties are a map of PropertyKey to Value
@@ -209,7 +209,8 @@ namespace mtconnect {
       /// @return `VALUE` property
       Value &getValue()
       {
-        static Value null;
+        thread_local Value null;
+        null = std::monostate {};
         auto p = m_properties.find("VALUE");
         if (p != m_properties.end())
           return p->second;
@@ -260,7 +261,8 @@ namespace mtconnect {
       /// @returns a reference to the entity list. Returns an empty list if it does not exist.
       EntityList &getListProperty()
       {
-        static EntityList null;
+        thread_local EntityList null;
+        null.clear();
 
         auto p = m_properties.find("LIST");
         if (p != m_properties.end())
@@ -451,7 +453,8 @@ namespace mtconnect {
 
       Value &getProperty_(const std::string &name)
       {
-        static Value noValue {std::monostate()};
+        thread_local Value noValue {std::monostate()};
+        noValue = std::monostate {};
         auto it = m_properties.find(name);
         if (it == m_properties.end())
           return noValue;
@@ -714,6 +717,11 @@ namespace mtconnect {
         }
       }
 
+      for (auto &key : removed)
+      {
+        m_properties.erase(key);
+      }
+
       return changed;
     }