STL thread safety

[Hunter2]

Hey all, I ran into a bug in my program relating to a std::set, where the set is accessed by two threads. I used a CRITICAL_SECTION to sync the threads and make sure that only one thread accesses the set at a time. But when I removed an element (the only element) from the set in one thread, then looped through the set in the other thread, I got an access violation even though everything was wrapped in calls to EnterCriticalSection() and LeaveCriticalSection(). I checked the code several times but coudn't see anything wrong; thinking it might be a MSVC bug, I checked this site, and found:

The header <xtree> (original 25 June 1998) presented here eliminates all need for thread locks and corrects a number of thread-safety issues for template classes map, multimap, set, and multiset.

Shouldn't the thread sync'ing from the critical sections have prevented any problems? Or is there something going on under the hood of these tree-based containers that delays a little before exploding (thus bypassing the critical section stuff).. or something? Or should I look altogether elsewhere for the problem?

Also, does the term 'thread lock' simply refer to sync stuff like mutexes and critical sections?

[kuphryn]

Post an example.

Kuphryn

[Hunter2]

Ok. Do note that I'm using simple wrapper classes for the thread sync objects (prefix of e means event, cs means critical section). Also, index is the set in question.

In the main thread:

csIndex.enter();
eUpdate.set();
eUpdateResponse.query(INFINITE);

index.erase(index.find(si));  //si is always contained in the set

csIndex.leave();
eUpdateResponse.query(INFINITE);

In thread:

//This is all in a loop
DWORD res = WaitForMultipleObjects(count, events, FALSE, INFINITE);
if(res == WAIT_FAILED)
{
	MessageBox(NULL, "Wait failed", "Error", MB_OK | MB_ICONSTOP);
	return 1;
}

res -= WAIT_OBJECT_0;
if(res == 0)	//Quit
{
	break;
}
else if(res == 1)	//eUpdate was set
{
	eUpdateResponse.set();
	delete[] events;

	csIndex.enter();
	eUpdateResponse.set();

	count = index.size() + 2;
	events = new HANDLE[count];
	events[0] = quitEvent;
	events[1] = eUpdate;
			
	HANDLE* eIt = events + 2;
	std::set<SockInfo*>::iterator iIt = index.begin();
	for(; iIt != index.end(); ++iIt, ++eIt)
		*eIt = (HANDLE)((*iIt)->notify);   //Access violation here
	
	csIndex.leave();
}

Also, (*iIt) always evaluates to a valid pointer, or at least should. There's a lot of extra stuff, since the events are part of a greater scheme of things, to sync. properly with other potential threads... but that's the gist of it.

[pablo615]

I don't know if changing this will fix anything, but doesn't waiting on an object ina critical section violate the idea of a critical section? In the main thread, perhaps only the erase should be in the critical section as that is the only code that is accessing a shared object.

In the meantime I'm still trying to get an exe based on your code to help ya out some more.

PK

[Hunter2]

but doesn't waiting on an object ina critical section violate the idea of a critical section? In the main thread, perhaps only the erase should be in the critical section as that is the only code that is accessing a shared object.

The reason I put the events inside the critical section is because I'm using the event to signal to the thread that I want it to update. If I put the event outside the critical section, there's a chance that the thread would enter the critical section and update before any changes have been made (and the main thread would meanwhile be waiting to enter the critical section while the thread is updating). So I enter the critical section to hold it down, set the 'update' event and then wait for the thread's "I got your message; I have stopped accessing all protected areas, and I'll wait for the critical section to become available before doing anything" event signal... then it removes the element, leaves the critical section so the thread can update, and waits for the "I'm done updating, you can go on now" event signal from the thread. When it gets the signal, it then leaves another critical section (I didn't show it in the code) so that other threads can signal an update to the thread.

I've been over that particular sequence so many times before, I don't want to think about it anymore.. but tell me if you spot a logic error