[GitHub] [nifi-minifi-cpp] nghiaxlee commented on a change in pull request #812: MINIFICPP-1247 - Enhance logging for CWEL

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

nghiaxlee commented on a change in pull request #812:
URL: https://github.com/apache/nifi-minifi-cpp/pull/812#discussion_r439750893



##########
File path: extensions/windows-event-log/ConsumeWindowsEventLog.cpp
##########
@@ -173,6 +173,15 @@ ConsumeWindowsEventLog::ConsumeWindowsEventLog(const std::string& name, utils::I
   writePlainText_ = false;
 }
 
+void ConsumeWindowsEventLog::notifyStop() {
+  logger_->log_trace("start notifyStop"); 
+  pBookmark_.reset();
+  if (hMsobjsDll_) {
+    FreeLibrary(hMsobjsDll_);

Review comment:
       I have not read the Load/FreeLibrary functions. I just realized that this processor haven't had a notifyStop yet, so I go ahead and create one.
   
   As a lesson from PublishKafka, things allocated in onSchedule should be released in notifyStop, because every time the agent receives a new configuration, it will create new processor objects instead of reusing those previous ones, and those previous instances will just stay inactive but not destruct (due to shared ptr leak?), @arpadboda please correct me if I misunderstand.
   
   @szaszm if you already knew the above issue or if you think that I misunderstand it, please let me know (and if it is feasible, a brief summary about Load/FreeLibrary would be great, but I will surely take a look at the docs as well).
   
   Since I'm unfamiliar with Windows API, I am not sure if the current implementation is intended, I will check the docs. In any case, your second point makes sense, thanks for the suggestion!




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