[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1284: MINIFICPP-1776 - Parallel heartbeat and flow update can cause crash

[GitBox <gi...@apache.org>]

fgerlits commented on a change in pull request #1284:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1284#discussion_r832014350



##########
File path: libminifi/src/FlowController.cpp
##########
@@ -438,6 +440,20 @@ std::shared_ptr<state::response::ResponseNode> FlowController::getAgentManifest(
   return agentInfo;
 }
 
+void FlowController::executeOnAllComponents(std::function<void(state::StateController*)> func) {
+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  for (auto* component: getAllComponents()) {
+    func(component);
+  }
+}
+
+void FlowController::executeOnComponent(const std::string &name, std::function<void(state::StateController*)> func) {
+  std::lock_guard<std::recursive_mutex> lock(mutex_);
+  if (auto* component = getComponent(name); component != nullptr) {
+    func(component);
+  }

Review comment:
       maybe add an error log if the component is not found?  I think that should never happen, and a log could be useful if it does

##########
File path: libminifi/src/FlowController.cpp
##########
@@ -523,14 +537,14 @@ void FlowController::getAllProcessorControllers(std::vector<state::StateControll
   }
 }
 
-void FlowController::getProcessorController(const std::string& name, std::vector<state::StateController*>& controllerVec,
+void FlowController::getProcessorController(const std::string& name, state::StateController*& controller,
                                           const std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>& controllerFactory) {
   auto* processor = root_->findProcessorByName(name);
-  auto& controller = processor_to_controller_[processor->getUUID()];
-  if (!controller) {
-    controller = controllerFactory(*processor);
+  auto& foundController = processor_to_controller_[processor->getUUID()];

Review comment:
       we should check if `processor` is null before dereferencing it

##########
File path: libminifi/include/FlowController.h
##########
@@ -234,7 +233,11 @@ class FlowController : public core::controller::ForwardingControllerServiceProvi
   std::chrono::steady_clock::time_point start_time_;
 
  private:
-  void getProcessorController(const std::string& name, std::vector<state::StateController*>& controllerVec,
+  std::vector<state::StateController*> getAllComponents();
+
+  state::StateController* getComponent(const std::string &name);
+
+  void getProcessorController(const std::string& name, state::StateController*& controller,
                               const std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>& controllerFactory);

Review comment:
       this could return the `controller` instead of using the slightly strange-looking `state::StateController*&` out parameter:
   ```suggestion
     state::StateController* getProcessorController(const std::string& name,
                                 const std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>& controllerFactory);
   ```

##########
File path: libminifi/src/FlowController.cpp
##########
@@ -523,14 +537,14 @@ void FlowController::getAllProcessorControllers(std::vector<state::StateControll
   }
 }
 
-void FlowController::getProcessorController(const std::string& name, std::vector<state::StateController*>& controllerVec,
+void FlowController::getProcessorController(const std::string& name, state::StateController*& controller,
                                           const std::function<std::unique_ptr<state::ProcessorController>(core::Processor&)>& controllerFactory) {
   auto* processor = root_->findProcessorByName(name);
-  auto& controller = processor_to_controller_[processor->getUUID()];
-  if (!controller) {
-    controller = controllerFactory(*processor);
+  auto& foundController = processor_to_controller_[processor->getUUID()];
+  if (!foundController) {
+    foundController = controllerFactory(*processor);
   }
-  controllerVec.push_back(controller.get());
+  controller = foundController.get();

Review comment:
       `controller` is dangling: `controllerFactory` returns a `unique_ptr` which gets destroyed at the end of the function.
   
   I think we should add the newly created `ProcessorController` to the `processor_to_controller_` map, both here and in `getAllProcessorControllers`.

##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -180,14 +178,18 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
             resp.write(response.str());
             stream->write(resp.getBuffer());
           } else if (what == "components") {
+            std::vector<std::pair<std::string, bool>> components;
+            update_sink_->executeOnAllComponents([&components](state::StateController* component){
+              components.emplace_back(component->getComponentName(), component->isRunning());
+            });
             io::BufferStream resp;
             resp.write(&head, 1);
-            const auto size_ = gsl::narrow<uint16_t>(update_sink_->getAllComponents().size());
-            resp.write(size_);
-            for (const auto &component : update_sink_->getAllComponents()) {
-              resp.write(component->getComponentName());
-              resp.write(component->isRunning() ? "true" : "false");
+            resp.write(gsl::narrow<uint16_t>(gsl::narrow<uint16_t>(components.size())));

Review comment:
       one `gsl_narrow` is enough :)
   ```suggestion
               resp.write(gsl::narrow<uint16_t>(components.size()));
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org