Re: implementing a thread-safe queue
On May 14, 10:29 am, itcecsa <itce...@gmail.com> wrote:
this is my implementation. feel free to give me commands.
My first question is: What is the purpose of this class? A
multi threaded queue usually decouples producer and consumer
threads. In that case you should reconsider your design for
these conditions:
1) significantly higher rate of pushes over pops
2) significantly higher rate of pops over pushes
In your design 1) eventually triggers a bad_alloc and
2) eventually triggers a ReadEmptyQueue exception.
Is this really what you want? Threads are fully asynchronous
and there is nothing to prevent 1 or 2 from happening.
Consider to block the call when either:
1) push() when size() > limit
2) pop() when empty()
By blocking the call that runs at a too high rate you prevent
the problem from happening. The queue will synchronize the
producer and consumer rates between some boundary limits.
Have a look at condition variables for how to implement
blocking calls based on some condition.
#include <pthread.h>
#include <deque>
#include <exception>
/* exception class for pop() with empty queue */
class ReadEmptyQueue : public std::exception {
public:
virtual const char* what() const throw() {
return "Queue is empty!";
}
};
template <class T>
class Queue {
private:
std::deque<T> *container; // container for the elements
Why a pointer to deque<T>? container could just be a value member,
this saves you explicit initialization and destruction.
pthread_mutex_t mutex; // mutex for sync the queue
Queue(const Queue& q); // disable copy constructor
Queue& operator= (const Queue& q); // disable assign operator
public:
// to make it simple, this is the only constructor for Queue instance
explicit Queue():mutex(PTHREAD_MUTEX_INITIALIZER), container(new
std::deque<T>())
{}
explicit only has a meaning for single argument constructors. It
should not be here.
// destructor
virtual ~Queue(){
delete container;
}
virtual indicates that your class is designed as a base class.
Is that really your intention?
// number of elements
typename std::deque<T>::size_type size() const {
return container->size();
}
This is not thread safe. A simultaneous push or pop call could be
modifying the containers size while you read it.
//is queue empty?
bool empty() const {
return container->empty();
}
This is not thread safe. A simultaneous push or pop call could be
modifying the containers size while you read its empty state.
// insert element into the queue
void push (const T& elem) {
pthread_mutex_lock( &mutex );
container->push_back(elem);
pthread_mutex_unlock( &mutex );
}
This is not exception safe. The push_back() or T's copy constructor
could throw and the lock will not be released. Make a RAII class to
represent the locked state of the mutex object, its destructor will
take care of the unlock no matter how push() ends. Better yet, use
boosts or C++0x's mutex object. It is portable and provides lock
objects.
// pop element from the top of the queue
T pop () {
pthread_mutex_lock( &mutex );
if (container->empty()) {
throw ReadEmptyQueue();
}
T elem(container->front());
container->pop_front();
pthread_mutex_unlock( &mutex );
return elem;
}
This is not exception safe for the same reasons as for push()
but also:
Your throw ReadEmptyQueue() does not release the lock.
The return statement might invoke a copy constructor
which might throw. In that case you have lost the
element you just popped of the queue. This is a direct
example from Sutter's Exceptional C++ books. His advice
is to design your queue as the STL does: separate the
front() and pop_front() primitives. Either can be made
exception safe, combined they can not. More
pragmatically I think a void pop(T& result) member could
work or else just accept the problem. Copies don't throw
for POD types.
};
Thanks!
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]