[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #777: MINIFICPP-1216 - Controller Services Integration test is unstable

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

bakaid commented on a change in pull request #777:
URL: https://github.com/apache/nifi-minifi-cpp/pull/777#discussion_r421151804



##########
File path: libminifi/src/FlowController.cpp
##########
@@ -248,9 +248,9 @@ int16_t FlowController::stop(bool force, uint64_t timeToWait) {
     this->timer_scheduler_->stop();
     this->event_scheduler_->stop();
     this->cron_scheduler_->stop();
+    thread_pool_.shutdown();

Review comment:
       @arpadboda During development of the new state storage at one point I remember shutting down the thread pool before disabling the controller services. While it was my gut feeling that there should be no requirement for the thread pool to be running (nor did I found one), nevertheless it caused the shutdown to fail and the whole agent to freeze on C2 reload for example.
   
   I was operating under the assumption that `this->root_->stopProcessing` will stop the processors. If that's not the case, `this->flow_file_repo_->stop();` and `this->provenance_repo_->stop();` that also currently come before the processors' running onTriggers could end would also be problematic (in the same manner as disabling the controller services: it's not a nullptr exception but the last onTrigger will not be able to use them).
   
   This forced `disableAllControllerServices` was done because we leak resources on flow reload and we can't have two RocksDbPersistableKeyValueStoreServices running at the same time, because the first locks the db: the new flow's one will fail. This is just a workaround, not a proper solution: a proper solution would be to refactor and fix flow reload.
   
   If `disableAllControllerServices` can run fine without the thread pool (again, it failed once, but it might have been just one state inbetween thread pool and C2 refactors that came and went while I was doing work on the state PR), then I agree that it would be the safer option to so, but this change has to be tested thoroughly on C2 reload and shutdown conditions.
   
   Also, in this case looking into the repo shutdowns would also make sense.




----------------------------------------------------------------
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.

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