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

[kylenix]

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]

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

[Peer Sommerlund]

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]

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]

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]

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

[Peer Sommerlund]

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]

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