Correctly initialized members.
ThreadQueue() {
pthread_mutex_init(&m_qmtx, NULL);
pthread_cond_init(&m_condv, NULL);
}
But you don't check the error codes.
The object is potentially created with invalid members. Use exceptions to gurantee the state of your object.
Destructor does not do anything useful.
~ThreadQueue() {
pthread_mutex_lock(&m_qmtx);
m_queue.clear();
pthread_mutex_unlock(&m_qmtx);
}
The destructr is supposed to clean resources.
It does not. You should destroy the mutex and condition variable in the destructor (reverse order of creation to be consistent).
Also locking before clearing the queue does not buy you anything. If at the point the destructor is called the object is still visible to other threads then it is going to break anyway. The queue is going to call clear in its own destructor so it is pointless doing it manually.
The call to signal should be inside the lock.
void push(T t_data) {
pthread_mutex_lock(&m_qmtx);
m_queue.push_back(t_data);
pthread_mutex_unlock(&m_qmtx);
pthread_cond_signal(&m_condv);
}
Since you should be using RAII for you locking anyway (See below) the signal is going to fall inside the lock stratergy unless you do something complex.
Also you should pass the parameter by const reference rather than value.
Normally front() is going to give you a reference to the front object.
T front() {
T ret;
pthread_mutex_lock(&m_qmtx);
while (m_queue.empty()) {
pthread_cond_wait(&m_condv, &m_qmtx);
}
ret = m_queue.front();
pthread_mutex_unlock(&m_qmtx);
return ret;
}
Returning a copy is probably not what you want (especially for anything interesting). But returning a reference is not really an option either as that opens you to situations where you have race conditions (you have a reference to the top object and another thread pops it (destroying it) just before you call a method).
So you need to provide a method for accessing the object while maintaining the lock (this will probably require the return of a wrapper object that maintains a lock on the queue) or alternatively removing this function.
Other non C++ things that should be addressed:
The mutex should be locked/unlocked using RAII. You are leaving your code open to exception handling problems. If an exception is generated by T
which you have no control over then you could leave your queue in an unusable state with the mutex locked and no way to unlock it (because the stack of the locker has been unwound past the unlock because of an exception).
Yes this is correct:
if (!m_queue.empty())
m_queue.pop_front();
But don't get into bad habbits. No using the braces can lead to maintenance problems on C/C++ code because of macros. Best practice dictates that you always use {}
for sub statements of if/while/for
.
To make the code easy to read when including the code in the interface declaration put the variables at the top. Thus you can read the code in with the context of the variables and knowing their type. If you interface declaration is just an interface (and the code is put in the source file rather than the header) then it is fine to put the private variables at the bottom.