class Main
{
public:
Main ()
{
//create thread and critical section here
}
~Main ()
{
}
void addEvent (event e)
{
//enter critical section
m_list.add (e);
//exit critical section
}
void handle ()
{
//loop
//enter critical section
//handle events
//end critical section
//endloop
}
private:
void handlePaintEvent () {}
std::vector <event> m_list;
Thread t;
CriticalSection cs;
static unsigned int CALLBACK _handle (void *arg)
{ ((Main*)arg)->handle (); }
};
data sharing among threads
Author
Hi everyone,
I'm finally converting my single threaded win32 engine into using multiple threads. Basically what I have is a class that contains an event list, and an execute function which will loop through all the events and execute these appropriately. How the events are handled are specified through the Main class, as in the follow snippet. This isn't my exact code, but shows a pretty rough outline of what is going on.
I create the thread with _beginthreadex and pass it the _handle callback function. The thread creates successfully, and it executes successfully. However, when handle tries to access the m_list, it shows as gibberish.
I know I'm missing something here, but I'm not sure what it could be? I'm currently allocating Main on the heap, should I allocate it someplace else? Does it need to be in a thread local storage? Thanks for the help!
*Edit: Thats a mistake, thanks for catching that kuphryn
-brad
[Edited by - Galapaegos on July 26, 2006 5:37:55 PM]
memory allocation
container is vector of event *. addEvent() inserts an object, not its memory address.
Kuphryn
container is vector of event *. addEvent() inserts an object, not its memory address.
Kuphryn
Quote:Original post by Galapaegos
I create the thread with _beginthreadex and pass it the _handle callback function. The thread creates successfully, and it executes successfully. However, when handle tries to access the m_list, it shows as gibberish.
Sorry, what exactly to you mean by 'it shows as gibberish'? Do you get an access violation when accessing your m_list, or does the list only seem to contain nonsensical event objects?
Bear in mind that since you're storing your events by value (and since you pass them to to your addEvent() method by value), your event class may need a copy constructor.
Quote:Original post by Galapaegos
I'm currently allocating Main on the heap, should I allocate it someplace else? Does it need to be in a thread local storage? Thanks for the help!
Doesn't matter. Heap storage is fine. You may want to post more code, especially the body of your handle() method.
Are you trying to share m_list across threads? If so, you need to use a mutex (Win32 calls them critical sections) whenever you want to access it. Otherwise this happens:
Thread 1 starts to add content to the vector. push_back() needs to reallocate memory, so it starts trashing memory inside the vector
Context switch
Thread 2 accesses an element in the vector. Reads invalid memory. Returns gibberish or crashes
Assuming the code is still running, Thread 1 finishes adding to the vector.
It gets even worse if both threads add/remove to/from the vector.
EDIT: Hmm, I should really start reading peoples posts more carefully. Still, it may be wortwhile double checking that you're in the critical section when you get gibberish returned.
It gets even worse if both threads add/remove to/from the vector.
EDIT: Hmm, I should really start reading peoples posts more carefully. Still, it may be wortwhile double checking that you're in the critical section when you get gibberish returned.
hmm your code actually looks ok to me. not the way i'd do things but the addEvent function says in comments the method is wrapped in a critical section and your thread function handle says in comments the loop contents are also wrapped in a critical section. the way your code is now it should protect the list, however, i wouldn't make a critical section that spans the entire loop's contents (kinda defeats the purpose of using threads). somebody's going to be waiting. if you can post more code, at least every bit where the list is accessed, it would help. did u also call InitializeCriticalSection before calling EnterCS and LeaveCS? that could also make your critical section calls fail.
void handle () { //loop //enter critical section //handle events //end critical section //endloop }Careful here, you can get into a dead-lock if one of your events tries to add another event to the list. Your code would enter the critical section in handle(), start handling the event, which calls addEvent and waits for a critical section that will never be released. Instead, do something like this:
void handle () { //loop //enter critical section //get event object from list //end critical section //handle event //endloop }As for why your list is getting corrupt I can't see anything wrong (assuming that you enter and leave the critical section correctly).
Quote:Original post by joanusdmentia
Careful here, you can get into a dead-lock if one of your events tries to add another event to the list. Your code would enter the critical section in handle(), start handling the event, which calls addEvent and waits for a critical section that will never be released.
Quote:From EnterCriticalSection on MSDN
After a thread has ownership of a critical section, it can make additional calls to EnterCriticalSection or TryEnterCriticalSection without blocking its execution. This prevents a thread from deadlocking itself while waiting for a critical section that it already owns. The thread enters the critical section each time EnterCriticalSection and TryEnterCriticalSection succeed. A thread must call LeaveCriticalSection once for each time that it entered the critical section.
So, the situation you described will not lead to deadlock.
Your second code snippet is still better, however, since it holds the critical section for less time, thus reducing the length of time for which other threads waiting on the critical section are blocked.
Author
Hi,
Thanks for the replies everyone. I'll try and go a little more in depth. Right now none of my events spawn more events, but I'll keep that in mind later on down the road. This is the flow that I have currently:
By gibberish, I mean no events are available. Actually, all the memory for the class doesn't point properly, and its not to any helpful addresses, its only 0xabababab, and 0x00000***.
This is the only place events are manipulated, so I'm going to put some checks around my critical section and see if its really being created.
-brad
Thanks for the replies everyone. I'll try and go a little more in depth. Right now none of my events spawn more events, but I'll keep that in mind later on down the road. This is the flow that I have currently:
void Main::handle (){ //grab the first event event e = m_list[0]; switch (e.type) { case UPDATE: onPaintEvent (e); break; } //remove event from list m_list.erase (m_list.begin ());}void Main::onPaintEvent (){ //do all rendering here.}By gibberish, I mean no events are available. Actually, all the memory for the class doesn't point properly, and its not to any helpful addresses, its only 0xabababab, and 0x00000***.
This is the only place events are manipulated, so I'm going to put some checks around my critical section and see if its really being created.
-brad
Quote:Original post by Galapaegos0xabababab is a fill value used by the CRT in debug mode which represents data past the end of allocated memory. 0x00000*** is very close to 0x00000000, which makes me think it's probably a null pointer, offset slightly (accessing a member variable 0x100 or more bytes from the start of the null parent object.
By gibberish, I mean no events are available. Actually, all the memory for the class doesn't point properly, and its not to any helpful addresses, its only 0xabababab, and 0x00000***.
In both cases, it sounds like there's a context switch halfway through a thread accessing the data.
i don't see any critical section code in your Main::handle function. the last code you posted is very thread unsafe. he he he, let's see everything or if you can prepare a heavily stripped but compilable version of your code that runs and still has the problem would be great.
i would do something like this. of course, assuming simple multithreading, an event is always in the list... if it's a producer-consumer/bounded buffer problem i.e. one thread produces events and one thread consumes events, this won't work because the consumer thread has to wait for the producer thread to put something in the list. so perhaps what you're getting is from m_list[0] is junk since the list is empty maybe (EDIT: which is what i think Steve in the above post was pointing at)?
[Edited by - yadango on July 27, 2006 11:05:47 AM]
i would do something like this. of course, assuming simple multithreading, an event is always in the list... if it's a producer-consumer/bounded buffer problem i.e. one thread produces events and one thread consumes events, this won't work because the consumer thread has to wait for the producer thread to put something in the list. so perhaps what you're getting is from m_list[0] is junk since the list is empty maybe (EDIT: which is what i think Steve in the above post was pointing at)?
void Main::handle(){ for(;;) { // grab and then remove the first event EnterCriticalSection(&cs); // could also use a mutex/monitor event e = m_list[0]; m_list.erase(m_list.begin()); LeaveCriticalSection(&cs); switch(e.type) { case(UPDATE) : { onPaintEvent(e); break; } } }}[Edited by - yadango on July 27, 2006 11:05:47 AM]
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
