szaszm commented on a change in pull request #812:
URL: https://github.com/apache/nifi-minifi-cpp/pull/812#discussion_r439756516
##########
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:
`LoadLibrary`/`FreeLibrary` are used to dynamically load and unload
DLLs, similarly to `dlopen`/`dlclose` on POSIX.
I didn't mean to question the validity of implementing `notifyStop`, thank
you for doing that. I didn't know the exact lifetime of processors and assumed
reuse of the objects. Not sure about `dlopen`, but in case of `LoadLibrary`,
repeated opens of the same DLL increment a reference count instead of loading
the library multiple times, so it's good news that we can only leak it once.
:slightly_smiling_face:
The reason I thought `FreeLibrary` on every reload is dangerous was that I
had to debug an issue in the past (on a different project) where a callback
registered from a DLL was called after the unloading of the DLL which caused
the DLL's instance of common types (e.g. `std::string`) to become inaccessible
and the program to crash in a hard-to-debug way when trying to call that code.
After looking further into our usage of this particular DLL, I noticed that
we only use it in `substituteXMLPercentageItems`, and only to pass the handle
to a WINAPI call (`FormatMessage` in particular), so unloading the DLL while
the processor is not running shouldn't be a problem here.
This means that the first point is a non-issue. However, as you also
recognized, the second point still stands, i.e. we should set the pointer to
`nullptr` after releasing the lib.
----------------------------------------------------------------
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:
[email protected]
