Anything wrong with this? wxThread, wxCondition problem Topic is solved

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
kylenix
In need of some credit
In need of some credit
Posts: 5
Joined: Tue Nov 14, 2006 10:19 pm

Anything wrong with this? wxThread, wxCondition problem

Post by kylenix » Tue Nov 14, 2006 10:27 pm

I wrote a program basically like this:

Code: Select all

#include <wx/wx.h>
#include <queue>
#include <iostream>
typedef std::queue<int> fifo;

class AThread : public wxThread
{
public:
  AThread();
  virtual void *Entry();

  //virtual void OnExit();
};

class BThread : public wxThread
{
public:
  BThread();
  virtual void *Entry();

  //virtual void OnExit();
};

fifo fifoa;
wxCriticalSection csfifoa;
wxMutex mutFifoALow,mutFifoAHigh;
wxCondition condFifoALow(mutFifoALow),condFifoAHigh(mutFifoAHigh);

bool fifoa_low()
{
  wxCriticalSectionLocker locker(csfifoa);
  return fifoa.size()<5;
}

bool fifoa_high()
{
  wxCriticalSectionLocker locker(csfifoa);
  return fifoa.size()>10;
}

int main()
{
  wxInitialize();
  AThread* ath=new AThread;
  BThread* bth=new BThread;

  ath->Create();
  bth->Create();


  ath->Run();
  bth->Run();

  wxSleep(10);
  std::cout<<"delete both threads"<<std::endl;
  ath->Delete();
  bth->Delete();
  std::cout<<"exit"<<std::endl;
  wxSleep(6);
  return 0;
}


AThread::AThread():wxThread()
{
}

void *AThread::Entry()
{
  int c=0;
  while(!TestDestroy()){
    if(fifoa_high()){
      std::cerr<<"A: wait !fifoa_high"<<std::endl;
      condFifoALow.Signal();
      mutFifoAHigh.Lock();
      condFifoAHigh.Wait();
      std::cerr<<"A: wait !fifoa_high end"<<std::endl;
    }
    csfifoa.Enter();
    fifoa.push(c++);
    csfifoa.Leave();
    if(!fifoa_low()){
      condFifoALow.Signal();
    }
  }
  return NULL;
}

BThread::BThread():wxThread()
{
}

void *BThread::Entry()
{
  int c;
  while(!TestDestroy()){
    if(fifoa_low()){
      std::cerr<<"B: wait !fifoa_low"<<std::endl;
      condFifoAHigh.Signal();
      mutFifoALow.Lock();
      condFifoALow.Wait();
      std::cerr<<"B: wait !fifoa_low ends"<<std::endl;
    }
    csfifoa.Enter();
    c=fifoa.front();
    fifoa.pop();
    std::cout<<c<<" \tfifo_size="<<fifoa.size()<<std::endl;
    csfifoa.Leave();
    if(!fifoa_high()){
      condFifoAHigh.Signal();
    }

  }
  
  return NULL;
}

It tries to sync two threads, A and B.
Thread A is writing to a FIFO (queue<int>) while B is reading from it.
I use wxCondition for sync purpose.
However, I found the above code behaves abnormally.
In particular, both threads will wait indefinitely (deadlock)
A sample output is like below:

Code: Select all

1046    fifo_size=10
1047    fifo_size=9
A: wait !fifoa_high
1048    fifo_size=8
1049    fifo_size=7
1050    fifo_size=6
A: wait !fifoa_high end1051     fifo_size=5
1052    fifo_size=4
B: wait !fifoa_low
A: wait !fifoa_high

dead here!!!
It seems impossible for both A and B waiting for each other, because I signal the other party before any waiting.

Help is highly appreciated.

Jorg
Moderator
Moderator
Posts: 3971
Joined: Fri Aug 27, 2004 9:38 pm
Location: Delft, Netherlands
Contact:

Post by Jorg » Wed Nov 15, 2006 8:31 am

Two things.. std::cout being thread safe is still debatable.

This is also questionable:

Code: Select all

    condFifoAHigh.Signal();
      mutFifoALow.Lock(); 
And:

Code: Select all

      condFifoALow.Signal();
      mutFifoAHigh.Lock();
What happens when they both signal the other is and they both run in a lock? The High and Low lock can be ran simultaneaously (in theory).

What you need to do is the following:

- Create a single object called Queue (not protect std::queue by both threads that is asking for trouble)
- In that single object Queue, protect EVERY entry to std::queue by a mutex, but use only ONE mutex
- Control the same object by both threads

You will see that the deadlock will not occur. You are actually using two mutexes that are not really mutually exclusive (what mutex stands for ;-) because they can both be locked and waited on simultaneously.

The general message is, when you protect functionality, only provide one entry to the functionality (e.g. encapsulate in a single object) and only use one mutex.

Regards,
- Jorgen
Forensic Software Engineer
Netherlands Forensic Insitute
http://english.forensischinstituut.nl/
-------------------------------------
Jorg's WasteBucket
http://www.xs4all.nl/~jorgb/wb

Peer Sommerlund
Knows some wx things
Knows some wx things
Posts: 43
Joined: Tue Jun 13, 2006 7:21 am
Location: Denmark
Contact:

Post by Peer Sommerlund » Wed Nov 15, 2006 8:59 am

It seems to me that you forget to unlock the mutexes. I would have used wxMutexLocker instead of manually calling Lock inside the code.

As Jorg writes, the intended use of condition and mutex is a bit different from this example.

A quick search on the net for a tutorial: http://developer.apple.com/documentatio ... Locks.html

.. dont be confused by the tutorial having examples for posix threads on Mac OS X. The theory is the same.

Regards,
Peer

kylenix
In need of some credit
In need of some credit
Posts: 5
Joined: Tue Nov 14, 2006 10:19 pm

Post by kylenix » Thu Nov 16, 2006 12:35 am

Thanks jorg.
However, I am not protecting std::queue with two mutexes.
I only protect it with one criticalsection, csfifoa.
The two mutexes, mutFifoALow and mutFifoAHigh are only used for the purpose of the condition, condFifoALow and condFifoBLow.
For every access to std::queue, I definitely lock the criticalsection csfifoa.
Lock mutFifoAHigh and lock mutFifoALow can happen simultaneously without problem, coz they are not trying to access any shared data at all.
They lock and then wait for the other party to signal.
I don't see any problem yet.

kylenix
In need of some credit
In need of some credit
Posts: 5
Joined: Tue Nov 14, 2006 10:19 pm

Post by kylenix » Thu Nov 16, 2006 12:39 am

Hi Peer,
According to the docs of wxWidgets
wxCondition::Wait
wxCondError Wait()
Waits until the condition is signalled.

This method atomically releases the lock on the mutex associated with this condition (this is why it must be locked prior to calling Wait) and puts the thread to sleep until Signal or Broadcast is called.
It seems to me that I don't need to release the lock on mutex manually.
Am I right?

Jorg
Moderator
Moderator
Posts: 3971
Joined: Fri Aug 27, 2004 9:38 pm
Location: Delft, Netherlands
Contact:

Post by Jorg » Thu Nov 16, 2006 8:03 am

Hmm well try to encapsulate anyway, and see if you can avoid the deadlock using as little wxConditions and wxCriticalSectionLocker objects. When it works, you can add more signalling. Also because it seems the wxCriticalSection, wxMutex and wxCondition are created globally I am not sure what will happen. So, to avoid, simplify and encapsulate..

- Jorgen
Forensic Software Engineer
Netherlands Forensic Insitute
http://english.forensischinstituut.nl/
-------------------------------------
Jorg's WasteBucket
http://www.xs4all.nl/~jorgb/wb

Peer Sommerlund
Knows some wx things
Knows some wx things
Posts: 43
Joined: Tue Jun 13, 2006 7:21 am
Location: Denmark
Contact:

Post by Peer Sommerlund » Thu Nov 16, 2006 10:39 am

As I understand the semantics of condition.wait, it will unlock the mutex while waiting for the condition AND THEN RELOCK IT when it is signalled.. Thus, the second time you manually call lock, it will be locked already.

The wxWidgets documentation is not clear on this, but try to read e.g. http://www-128.ibm.com/developerworks/library/l-posix3/

kylenix
In need of some credit
In need of some credit
Posts: 5
Joined: Tue Nov 14, 2006 10:19 pm

Post by kylenix » Thu Nov 16, 2006 10:31 pm

Thanks Peer,
You've got the point.
Yes, I believe u r right.
When I unlock the mutex after wait, everything goes fine.
Thanks again

Post Reply