Shared counter variable among different threads

Question

I have a need to use a shared variable among different threads in c/C++. So putting my knowledge of threads and mutexes I have written the below sample code. Can anyone please review the code and provide me review comments so that I can improvise it in a more better way. Also I know about atomic variables but somehow I don't want to get into that and wanted to stick with threads & mutexes and locking mechanism. Below code is a working code.

#include <iostream>
#include <unistd.h>
#include <pthread.h>
using namespace std; 
class Thread
{
 public:
 Thread();
 virtual ~Thread();
 int start();
 int join();
 int detach();
 pthread_t self();
 virtual void* run() = 0;
 private:
 pthread_t m_tid;
 int m_running;
 int m_detached;
};
static void* runThread(void* arg)
{
 return ((Thread*)arg)->run();
}
Thread::Thread() : m_tid(0), m_running(0), m_detached(0) {}
Thread::~Thread()
{
 if (m_running == 1 && m_detached == 0) {
 pthread_detach(m_tid);
 }
 if (m_running == 1) {
 pthread_cancel(m_tid);
 }
}
int Thread::start()
{
 int result = pthread_create(&m_tid, NULL, runThread, this);
 if (result == 0) {
 m_running = 1;
 }
 return result;
}
int Thread::join()
{
 int result = -1;
 if (m_running == 1) {
 result = pthread_join(m_tid, NULL);
 if (result == 0) {
 m_detached = 0;
 }
 }
 return result;
}
int Thread::detach()
{
 int result = -1;
 cout<<"Detaching thread"<<endl;
 if (m_running == 1 && m_detached == 0) {
 result = pthread_detach(m_tid);
 if (result == 0) {
 m_detached = 1;
 }
 }
 return result;
}
pthread_t Thread::self() {
 return m_tid;
}
class Mutex
{
 friend class CondVar;
 pthread_mutex_t m_mutex;
 public:
 // just initialize to defaults
 Mutex() { pthread_mutex_init(&m_mutex, NULL); }
 virtual ~Mutex() { pthread_mutex_destroy(&m_mutex); }
 int lock() { return pthread_mutex_lock(&m_mutex); }
 int trylock() { return pthread_mutex_trylock(&m_mutex); }
 int unlock() { return pthread_mutex_unlock(&m_mutex); } 
};
class shared_integer { 
private: 
 int i;
 Mutex mlock;
public: 
 // Parameterised constructor 
 shared_integer(int i = 0) 
 { 
 this->i = i; 
 } 
 // Overloading the postfix operator 
 void operator++(int) 
 { 
 mlock.lock();
 (this->i)++; 
 mlock.unlock();
 } 
 void operator--(int) 
 { 
 (this->i)--; 
 } 
 // Function to display the value of i 
 void display() 
 { 
 cout << "Value became:" <<i << endl; 
 } 
}; 
class MyThread:public Thread 
{
Mutex mlock;
shared_integer &wd; 
public:
MyThread( 
 shared_integer & workd
 ):
 wd(workd){}
void *run() {
 for (int i = 0; i < 10; i++) {
 cout<<"thread "<<(long unsigned int)self()<<endl;
 //Currently testing with only work done
 wd++;
 wd.display();
 sleep(1);
 }
 cout<<"thread done "<<(long unsigned int)self()<<endl;
 return NULL;
 }
};
// Driver function 
int main(int argc , char ** argv) 
{ 
 shared_integer workdone(0); 
 MyThread* thread1 = new MyThread(workdone);
 MyThread* thread2 = new MyThread(workdone);
 MyThread* thread3 = new MyThread(workdone);
 MyThread* thread4 = new MyThread(workdone);
 cout<<"main After creating threads"<<endl;
 thread1->start();
 thread2->start();
 thread3->start();
 thread4->start();
 cout<<"main Before joining first therad"<<endl;
 cout<<"main Before joining second therad"<<endl;
 thread1->join();
 thread2->join();
 thread3->join();
 thread4->join();
 cout<<"main done"<<endl;
return 0;
}

• ALX23z: Why not just use C++11 std::thread? Or C++11 atomic variables and mutexes?
• user3629249: the posted code is NOT c

Answer 1

If I would get this code at my job, I would reject it. There is NO valid reason for using the pthread library directly to manage threads. Please use std::thread instead. Normally, I wouldn't even do the effort to read further. The same holds for the mutex, just use std::mutex, std::shared_mutex ... instead.

I see your usage of shared_integer, which raises a few questions: - Isn't this std::atomic<int>, less performant? (Yes, I know you don't want to get into it, though, I would still recommend looking into it if you don't expose the mutex) - Secondly, your implementation is flawed, as operator-- and display() don't use the lock - Thirdly, I would urge you to make this a template. It's easy to make mistakes into this, you only want to have that kind of code once.

Looking at your main-function, it looks like you have a memory leak.

The same code using the C++ standard library:

#include <thread>
#include <iostream>
#include <chrono>
#include <atomic>
int main(int argc , char ** argv) 
{ 
 std::atomic<int> workdonecount{0}; 
 auto workdone = [&workdonecount]()
 {
 for (int i = 0; i < 10; i++) {
 std::cout<<"thread "<<std::this_thread::get_id()<<std::endl;
 //Currently testing with only work done
 workdonecount++;
 std::cout << "Value became:" << workdonecount << std::endl;
 std::this_thread::sleep_for(std::chrono::seconds{1});
 }
 std::cout<<"thread done "<<std::this_thread::get_id()<<std::endl;
 };
 std::cout<<"main After creating threads"<<std::endl;
 auto thread1 = std::thread(workdone);
 auto thread2 = std::thread(workdone);
 auto thread3 = std::thread(workdone);
 auto thread4 = std::thread(workdone);
 std::cout<<"main Before joining first therad"<<std::endl;
 std::cout<<"main Before joining second therad"<<std::endl;
 thread1.join();
 thread2.join();
 thread3.join();
 thread4.join();
 std::cout<<"main done"<<std::endl;
return 0;
}

Code at compiler explorer using c++17

From C++20, you could even use std::jthread and let the threads join automatically.

PS: This code contains the same bug that you have:

 workdonecount++;
 std::cout << "Value became:" << workdonecount << std::endl;

Should become:

 auto newValue = ++workdonecount;
 std::cout << "Value became: " << newValue << std::endl;