Threading and STL/Runtime problems

[Novative]

Hello out there,

I have a little multithreaded application watching changes in the filesystem. Setting up the threading/synchronization part was actually really easy, unfortunately, I regularly (means in about 60% of all executions) run into problems on shutting down my application, segfaulting in stl destructors (like e.g of std::string) or other obscure parts of the runtime, mostly stating "Free Heap block XXX modified at XXXX after it was freed". I can rule out bad programming on my side to about 90%.

Is there any danger involved in doing thread cleanup code in a C++ destructor ? Or is there anything fundamental I overlooked (like the need for a special thread safe variant of the stl, though I explicitely took care of synchronization of the containers involved) ?

Any hints and/or suggestions appreciated,

Thanks in advance,

Novative

[wildfrog]

You need to make sure that a thread is done with a given resource, before you attempt to clean up/free that resource.

- petter

[Novative]

Thanks for the suggestion,

I did so by doing an infinite WaitForSingleObject on the thread handle in the destructor of the class that holds my thread handle (their lifetime is willingly bound). The thread gets notified about the shutdown via an event, clears up ressources and leaves the thread function.

The destruction of the membervariables of that containing class often fails for no particular reason I could understand. My structure is about this (in pseudocode)



//WARNING THIS IS PSEUDOCODE

class RessourceManager
{

~RessourceManager()
{
SetEvent(m_shutdownevent);
WaitForSingleObject(m_threadhandle);

foreach entry in list
delete entry-pointer;

CloseHandle(m_threadhandle);
CloseHandle(m_shutdownevent);

//this is the place where 90% of the errors occur,
//i.e. on cleanup of the member variables of ressource manager
}


static void threadfunction(void* pointertomanager)
{
RessourceManager* that = (RessourceManager*)pointertomanager;
while(true)
{
HANDLE* handles = new HANDLE[handlecount];
DWORD event = WaitForMultipleObjects(handles);
if(event == that->m_shutdownEvent)
{
delete [] handles;
_endthread();
}

// further processing .....
delete [] handles;
}
}

std::list<SomeData*> m_shareddata;
HANDLE m_threadhandle;
HEVENT m_shutdownEvent;
}



So if there is per se nothing wrong in using STL and the runtime functions (esp. delete) as usual (i.e. in a non multi-threading situtation), I might be the only one left to blame ;)

Has anyone experienced similar problems ?

Thanks again,

Novative

[wildfrog]

The pseudocode won't help much in finding any bugs.

So if there is per se nothing wrong in using STL and the runtime functions (esp. delete) as usual (i.e. in a non multi-threading situtation), I might be the only one left to blame

Has anyone experienced similar problems ?Yes, I've experience similar problems, and I'm 99% sure that this problem relates to your code.


HANDLE* handles = new HANDLE[handlecount];
Do you need to store the handles on the heap? IMO an array on the stack would be cleaner.

- petter

[Novative]

Do you need to store the handles on the heap? IMO an array on the stack would be cleaner.


I need to deal with a variable number of ressource handles, thats why its not initialized on the stack, otherwise I'd gladly agree with you.



The pseudocode won't help much in finding any bugs.

Sorry, that was just to make sure, there has been no conceptual error in the algorithm before obfuscating the underlying idea by code.

I'll post the code in a second, note however that this is prototyping code, so there is a lot of room for optimization and making things prettier/better than they are.

Thanks for delving in,
Novative



struct HandleInfo
{
HANDLE Handle; // the handle of the directory obtained by CreateFile
URL Directory; //the path of the directory
OVERLAPPED Overlapped; // overlapped structure for use in ReadDirectoryChangesW
int Counter; //how many times is this handle opened ?
};

ResourceManager::~ResourceManager()
{
//notify thread about shutdown
SetEvent(m_shutdownevent);

//do an infinite wait on the thread handle
//this will unblock when that thread has finished
WaitForSingleObject(m_workerthread, INFINITE);

//cleanup memory used by the HandleInfos
EnterCriticalSection(&m_criticalsection);
std::list<HandleInfo*>::iterator it = m_handles.begin();
while(it != m_handles.end())
{
//Note that only the memory is freed,
//the element itself is kept in the list
//this will be automatically cleaned on list destruction
delete *it;
++it;
}
LeaveCriticalSection(&m_criticalsection);


//Cleanup ressources
CloseHandle(m_shutdownevent);
CloseHandle(m_workerthread);

DeleteCriticalSection(&m_criticalsection);
}



void ResourceManager::workerthread(void* data)
{
ResourceManager* that = (ResourceManager*) data;

EnterCriticalSection(&that->m_criticalsection);

//Register every handle asynchronously with ReadDirectoryChangesW
std::list<HandleInfo*>::iterator it = that->m_handles.begin();
while(it != that->m_handles.end())
{
int success = ReadDirectoryChangesW((*it)->Handle,
that->m_notifyinfo,
sizeof(that->m_notifyinfo),
FALSE,
FILE_NOTIFY_CHANGE_LAST_WRITE,
&that->m_byteswritten,
&(*it)->Overlapped,
NULL);

++it;
}

LeaveCriticalSection(&that->m_criticalsection);

//you get notified twice by ReadDirectoryChangesW when a file changes
//this little flag prevents this
std::string ghosting;

while(true)
{

//Build handles array
EnterCriticalSection(&that->m_criticalsection);
//one more for the shutdown handle
size_t handlesize = that->m_handles.size()+1;
HANDLE* handles = new HANDLE[handlesize];
unsigned int i = 0;

it = that->m_handles.begin();
while(it != that->m_handles.end())
{
handles[i++] = (*it)->Overlapped.hEvent;
++it;
}

//add the shutdown handle as the last entry
handles[i] = that->m_shutdownevent;
LeaveCriticalSection(&that->m_criticalsection);


//Wait for somehting to happen
DWORD result = WaitForMultipleObjects((DWORD)handlesize, handles, FALSE, INFINITE);

//Did we hit the shutdownevent ?
if(result == handlesize-1)
{
//dont forget the handles
delete [] handles;
ExitThread(0);
}

if(result >= WAIT_OBJECT_0 && result < WAIT_OBJECT_0 + handlesize)
{
int entry = 0;

FILE_NOTIFY_INFORMATION* info = that->m_notifyinfo;

HandleInfo* handleinfo = 0;

do
{
//find matching handleinfo to the event that occured

//extract the file and pathname
EnterCriticalSection(&that->m_criticalsection);

std::map<std::string, Resource*>::iterator found = that->m_resources.find(path);
if(found != that->m_resources.end())
{
//reload the resource
found->second->Reload();
}


LeaveCriticalSection(&that->m_criticalsection);

info = (FILE_NOTIFY_INFORMATION*) ((char*)info + info->NextEntryOffset);
}
while(info->NextEntryOffset != 0);

//setup this handle for a new change notification
int error = ReadDirectoryChangesW(handleinfo->Handle,
that->m_notifyinfo,
sizeof(that->m_notifyinfo),
FALSE,
FILE_NOTIFY_CHANGE_LAST_WRITE,
&that->m_byteswritten,
&handleinfo->Overlapped,
NULL);

}

//cleanup the handles
delete [] handles;
}

[JVene]

I'm very familiar with this problem.

It comes in flavors ranging from STL, GUI, and even my own objects.

The problem is shutdown coordination - that is, the order of events choreographed to close down everything in an order that doesn't violate dependencies.

I generally construct an application in rings representing the dependency relationships, such that I can back out from the 'center' of the ring (the imaginary position in which the application runs).

Typically, if there's any global critical sections or other global objects upon which threaded material takes action, you run into problems like this.

You can't guarantee the order of destruction in global objects, including critical sections. In some builds, the order might just happen to be ok, but then in another build, after expansion of the code, it crashes on exit.

If ever I have the concept of a global value protected by a critical section, or any other global resource (usually application configuration data, or a thread scheduler/synchronizer related to the number of processors in the machine), my tactic involves the use of a 'global smart pointer'.

These 'pointers' are managed by a 'global manager' - the outer ring, if you will, of my application's design. I never let the objects reside in the global scope itself, only these smart pointers. The only exception is a framework application object, which the frame usually expects is a global object.

The smart pointer I'm talking about is a specialization on smart pointers. They are pointers which start out as null, but upon first use - that is, any runtime code that dereferences that pointer with, say:

someThing->CallFunc();


the smart pointer recognizes that this object has not yet been uninitialized, and creates the appropriate object to complete the call. The pointer is from global scope, the object is on the heap - and these objects are ALL registered with a global pointer manager (at runtime).

At application shutdown, I call a shutdown function in that global manager, and it destroys all of these objects during shutdown, as processed by a shutdown message (WM_CLOSE, for example). This completes before the rest of the general runtime exit, where global objects would otherwise be destroyed as a result of return from the main (winmain, whatever) function.

With this approach, I generally don't have shutdown issues, with the possible exception of the order in which these 'global object pointers' are destroyed, for which I have a provision for setting a 'level' when creating the pointer, to control that order.

Now, this system is overkill for many application designs - I use it because I built it years ago, it's debugged and solid. For you, simply taking care to ensure the shutdown order violates no dependencies goes a long way to stopping this problem. Quite often what you'll find is a basic design notion will avoid the problem entirely, like placing everything global inside an object you can shut down at the WM_CLOSE ( or similar) stage.