-
Notifications
You must be signed in to change notification settings - Fork 567
Add LOG4CPLUS_NO_AUTO_DESTROY option. #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.0.x
Are you sure you want to change the base?
Conversation
This adds a CMake option LOG4CPLUS_NO_AUTO_DESTROY which disables automatically destroying the default logging context. This fixes an issue on Windows where the application hangs when exiting abnormally such as upon CTRL+C.
Are you using the asynchronous logging feature? If you are not using it, you can disable the threadpool entirely. The option is the one below your new patched option. |
I thought about this and I think this is incorrect way of handling CTRL+C on Windows. I think that what you need to do is to install a handler using |
Hi, According to the Windows documentation, the default CTRL-C handler calls ExitProcess. So it must be that the issue will actually appear when calling ExitProcess in general. Is not good for a library to break such an essential function. The ExitProcess documentation explains that all threads are immediately terminated, as I have suspected. It is only after that that DLL_PROCESS_DETACH are called. I do not know exactly what the latter involves, but I suspect it includes destruction of global objects. So it is then true that trying to shut down the thread pool in this context is not appropriate to do. If it would be possible to get a notification in the context of DLL_PROCESS_DETACH before global objects are destructed, then we would be able to set some flag and prevent shutting down the thread pool only in that case. Actually, this place seems to be the existing thread_callback_terminator. So, for example, if we just add code there to set default_context to null in the case of DLL_PROCESS_DETACH only (not DLL_THREAD_DETACH), that may already solve the problem (but leave a small memory leak which is probably not really a problem). |
Here's a blog post from a Microsoft developer that explicitly states that applications should not do any cleanup in DLL_PROCESS_DETACH: https://devblogs.microsoft.com/oldnewthing/20120105-00/?p=8683 IF lpReserved is indicating that the process is exiting. So the suggested inhibition of cleanup should be conditional on that. |
OK, we could improve the DLL_PROCESS_DETACH handling by not doing thread cleanup when
Log4cplus is a complex library with its own necessary initialization and deinitialization. If the default handler is not good enough then you have to provide your own. You should probably never call |
I have experienced the the same hang on exiting. Same as you in that I was using log4cplus.DLL in another DLL (and in EXE too). The following code fixes the problem (note: I put this code in version 2.0.4, so may need adjusting for your specific version): inline ThreadPool::~ThreadPool()
{
std::unique_lock<std::mutex> lock(queue_mutex);
stop = true;
condition_consumers.notify_all();
condition_producers.notify_all();
pool_size = 0;
condition_consumers.wait(lock,
[this]
{
//return this->workers.empty();
if (this->workers.empty()) {
return true;
} else {
#if defined (_WIN32)
for (std::vector< std::thread >::reverse_iterator i = this->workers.rbegin(); i < this->workers.rend(); ++i) {
HANDLE hThread = i->native_handle();
if (WaitForSingleObject(hThread, 0) != WAIT_OBJECT_0) {
return false;
}
}
for (std::vector< std::thread >::reverse_iterator i = this->workers.rbegin(); i < this->workers.rend(); ++i) {
i->detach();
}
return true;
#else
return false;
#endif
}
});
assert(in_flight == 0);
} PS: I expect this to work in all cases (ie: static lib or DLL linked into another DLL or EXE) but have not tested it. OK, I guess I am testing log4cplus.DLL being used from several DLLs and EXE. I also have tested log4cplus.DLL being used from a dynamically loaded DLL into same EXE and all worked great. |
@ambrop72 Have you given my solution a try? My solution should work with log4cplus version 2.0.2 - 2.0.7 as drop-in replacement for |
We have experienced a problem that when log4cplus is used on Windows (with Visual Studio) as a DLL or statically linked into a DLL, that if the application is terminated by CTRL+C while log4cplus is initialized, the application hangs instead of exiting. Using the debugger revealed that it was hung in the destructor of
destroy_default_context
and further in theThreadPool
destructor, and that there was only a single thread at that time. Based on this observation, I think that Windows aborts all threads but still runs the static destructors. And of course theThreadPool
destructor hangs as it tries to synchronize with threads that no longer exist.The only solution I could think of is to not destruct the default context from a static destructor. Therefore, this patch adds a new CMake option
LOG4CPLUS_NO_AUTO_DESTROY
, which if enabled, disables thedestroy_default_context
code. This completely solves the problem for us.