Add LOG4CPLUS_NO_AUTO_DESTROY option.

[ambrop72]

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 the ThreadPool 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 the ThreadPool 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 the destroy_default_context code. This completely solves the problem for us.

[wilx]

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.

[wilx]

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 SetConsoleCtrlHandler and handle the log4cplus deinitialization trigger via this handler.

[ambrop72]

Hi,
Thanks for your response. We are not (to our knowledge) using asynchronous logging, we only configure ConsoleAppender using log4cplus.properties. So we will try to disable the thread pool instead. Still I offer some comments about the issue below, so that maybe a complete fix can be found that makes things work by default.

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

[ambrop72]

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.

[wilx]

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 lpvReserved != NULL. But that does not solve your case. The cleanup is called as part of normal C++ object lifetime cycle.

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.

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 ExitProcess() explicitly but you should instead go through C++'s std::exit() or std::quick_exit() and also deinitialize Log4cplus properly before that.

[sldr]

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.

[sldr]

@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 inline ThreadPool::~ThreadPool(). If you are using log4cplus version 2.0.0 or 2.0.1, then it may need a bit of modification (let me know n I will get my code to work with 2.0.0 or 2.0.1). If you are using master (aka 3.0.0) it is a drop-in replacement (at current, but that could change at anytime).