Open CurlyMoo opened 9 years ago
You should be calling Manager::Get()->RemoveDriver(
That will gracefully send any queued up notifications etc and should terminate the threads you are seeing hanging around. (Thread->Stop() will signal the thread to exit, hang around for 2 seconds if it doesn't gracefully exit and then call Thread->Terminate())
if doing it that way, I dont see any leaks in valgrind.
(A Manager::Destroy() should be called after that - It forcefully "destroys" everything without giving much of a change for anything to cleanup or flush queues etc).
You should be calling Manager::Get()->RemoveDriver() when shutting down before the Manager::Destroy() (but that leads to a race condition sometimes)
Why is there actually a possibility of a race condition? Shouldn't you be sure there can't be any? I don't feel invited to call RemoveDriver if there is this chance.
(Thread->Stop() will signal the thread to exit, hang around for 2 seconds if it doesn't gracefully exit and then call Thread->Terminate())
Again, you should just know when a thread is stopped not give it a certain time before it has to stop. Even if that takes 3 seconds. If you think it could be still hanging around after 2 seconds then there is some fault in the threaded function. IMHO programmers should never need a pthread_cancel
except in absolute emergency situations.
My example clearly shows how this could be properly implemented.
The race condition exists on the boundry between OZW and the application. Try to follow me here: Notifications are actually Queued up and sent the Application by the main loop in the Driver Thread. When we shutdown the Driver Thread, we flush the Notification Queue, and all the Notifications get delivered to the application (#1), then the driver thread continues to deallocate as well. If any of the Application Notification Handlers call back to the Manager::Get()->whatever functions while the Driver Thread is being destroyed (eg, you do a Manager::Get->GetValueIDLabel or whatnot) then the Driver Class (and thread) could be in various stages of destruction and either throw a exception (HomeID doesn't exist) or return "incomplete" data.
So ideally (as you have done above) is to remove the Notifications before destroying the Driver Thread, but then you would manually have to remove nodes etc from your application rather than handling the NodeRemoved Notifications etc.
As for the timeout - Welcome to the wonderful world of cross platform, where we don't have the luxury of coding for just Linux. The generic code has to deal with Windows threading as well... (and there is some work on Android that could further complicate the threading classes in the future).
As Note 2 indicated above, the reason we give 2 second timeouts here before forcefully destroying the thread is because we are event driven to the application via Callbacks, writing out config files, flushing buffers etc. The timeout is to ensure that we never "block" because of a poor implementation. I havn't seen anything that actually takes longer than 2 seconds though, so its just a "it works for us" number.
So ideally (as you have done above) is to remove the Notifications before destroying the Driver Thread, but then you would manually have to remove nodes etc from your application rather than handling the NodeRemoved Notifications etc.
Ok, the removal of nodes is not a problem, but in my implementation the memory issue still occurs. You say that is fixed by calling Manager::Get()->RemoveDriver()
before OpenZWave::Manager::Destroy();
? As long as the Manager isn't used afterwards no race condition should occur.
As for the timeout - Welcome to the wonderful world of cross platform, where we don't have the luxury of coding for just Linux.
I use winpthread (on Windows) in my cross-platform applications and have good experience with it.
Version:
2015-05-28 23:26:31.852 Always, OpenZwave Version 1.2.919 Starting Up
Relevant code snippets:
Initialize:
Deinitialize:
Adding a small check in
bool ThreadImpl::Terminate
tells me that this function is never called. There is another issue in this function, but i will get to that later.To confirm the (possible) leak, i added a
pthread_detach
after thepthread_create
inbool ThreadImpl::Start
making the join obsolete. This resolves the leak altogether.So the issue here is either:
bool ThreadImpl::Terminate
not being called.bool ThreadImpl::Terminate
.There is however still another issue in the code. A
pthread_cancel
followed by apthread_join
can leak memory when the running thread is not gracefully stopped (and could not garbage collect its own processes). The Thread class should somehow watch if a thread is terminated before thepthread_join
is called.A small snippet how i do this in C: