MultiThreading producer/consumer

Question

I'm writing a basic producer/consumer threading code. I got most of it to work, but I'm having an issue with rand(). Or maybe my issues are deeper than rand(), and rand is only the tip of the iceberg. I don't want to do anything too complex, (none of the runnable or wait).

I have a global deque of integers that acts as the buffer. I allow the user to enter in the size and the limit of run times. I make the counter a static global variable. this is my producer:

DWORD WINAPI producer(LPVOID n)
{
  cout << "\nPRODUCER:The producer is running right now" << endl;
  int size = (int)n;
  int num = rand()%10;// this is for making item.
  while (buffer.size() > size)
    {
    }

  buffer.push_back(num);
  counter++;
  return (DWORD)n;

}

this is my consumer--

 DWORD WINAPI consumer(LPVOID n)
 {

   cout << "\nCONSUMER:The consumer is running right now" << endl;
   while (buffer.empty())
     { }
   int item= buffer.front();
   cout << "\nCONSUMER:The consumer ate" << item << endl;
   counter++;

   return (DWORD)n;


  }

in main-

 while (counter < l)
  {
     hThreads[0] = CreateThread(NULL, 0, producer, (LPVOID)s, NULL, &id[0]);
     hThreads[1] = CreateThread(NULL, 0, consumer, (LPVOID)l, NULL, &id[1]);

     waiter = WaitForMultipleObjects(MAX_THREADS, hThreads, TRUE, INFINITE);
   }
  for (int i = 0; i < MAX_THREADS; i++) {
     CloseHandle(hThreads[i]);
   }

My output is like this:

So it only generates 1 every time. Srand didn't work either. But it runs the correct number of times.

EDIT--- So I fixed producer and consumer to deal with the race condition:

  DWORD WINAPI producer(LPVOID s)
  {
     WaitForSingleObject(Empty, INFINITE);    
     WaitForSingleObject(Mutex, INFINITE);
     cout << "\nPRODUCER...." << endl;
     int size = (int)s;
     srand(size);
     int in = rand() % 10;
     cout << "THIS IS IN:::" << in << endl;
     while (buffer.size() == size)
     {
        ReleaseMutex(Mutex);
      }
     buffer.push_back(in);
     counter++;
     cout << "\nThe producer produces " << buffer.front() << endl;
     ReleaseMutex(Mutex);
     ReleaseSemaphore(Full, 1, NULL);

     return (DWORD)s;
     }



     DWORD WINAPI consumer(LPVOID l)
     {
        WaitForSingleObject(Full, INFINITE);   
        WaitForSingleObject(Mutex, INFINITE);
        cout << "\nCONSUMER...." << endl;
        while (buffer.empty())
       {
             ReleaseMutex(Mutex);

       }
    int out = buffer.front();
    counter++;
    ReleaseMutex(Mutex);
    ReleaseSemaphore(Empty, 1, NULL);
    return (DWORD)l;
    }

BUT the random thing still keeps acting up. It only keeps producing one number over and over (even when seeded).

Answer 1

Yes, it doesn't make sense to create (and destroy) a thread to only create or process one number - the extra overhead just isn't worth. Plus your code (as is) has some quite apparent errors or misconceptions. These are:

• In the main thread you create the worker threads (in that while(){ } loop) but only destroy them once at the end, that is you destroy only the handles created in the last loop.
• As noted in my message, srand() is called for each number to be generated, and always with the same initial seed, so it's normal to get the same numbers.
• The while() loops checking whether the buffer is empty or full don't make sense, and should not release the Mutex.
• The operations on the counter variable are probably wrong. Both the producer and consumer threads increase it, and the main thread uses it to determine how many numbers to generate/print.
• In your initial code fragment, counter is a global variable, with more than one threads operating on it, so you should read or modify it in a thread-safe manner, not this way. You should use some locking mechanism, like a critical section or interlocked variable access.

---

My suggestion would be to create one producer thread (generating all numbers) and one consumer thread (printing all numbers), transferred through the buffer. For this, you will need the following items (you have already implemented most of them):

• A "Full" semaphore, counting the numbers in the buffer, initially 0.
• A complementary "Empty" semaphore, counting the empty items in the buffer, initially set to the buffer size. The "sum" of these semaphores should of course always be equal to the buffer size.
• A mutex (or critical section), used for accessing the buffer.
• A global variable, used for telling threads whether to exit.

I'm posting some code samples below. Not sure if they work, you may need to revise or debug them, but this is just for showcasing the concept:

// Global
#define MAX_BUF 5
BOOL bExit = FALSE;

// Main Thread
    Empty = CreateSemaphore(NULL, MAX_BUF, MAX_BUF, NULL);
    Full = CreateSemaphore(NULL, 0, MAX_BUF, NULL);
    .
    .
    hThreads[0] = CreateThread(NULL, 0, producer, (LPVOID)l, NULL, &id[0]);
    hThreads[1] = CreateThread(NULL, 0, consumer, (LPVOID)l, NULL, &id[1]);
    waiter = WaitForMultipleObjects(MAX_THREADS, hThreads, TRUE, INFINITE);

    for (int i = 0; i < MAX_THREADS; i++)
        CloseHandle(hThreads[i]);


DWORD WINAPI producer(LPVOID nCount)
{
    int nItems = (int)nCount;
    // Initialize rand() seed - each run will be generating a different sequence
    srand(GetTickCount()); // May need to AND GetTickCount() with RAND_MAX ???
    // Generate nCount numbers
    for (int i = 0; i < nItems; i++)
    {
        if (bExit) return 9; // Aborted
        WaitForSingleObject(Empty, INFINITE); // Wait until at least one item empty
        // Lock the buffer and add an item
        WaitForSingleObject(Mutex, INFINITE); // Could be EnterCriticalSection() instead
        if (buffer.size() >= MAX_BUF)
        {
            cout << "\nInternal Error: Buffer-full Check Failed!" << endl;
            bExit = TRUE; // Tell all threads to exit
            ReleaseMutex(Mutex); 
            return 1; // Exit with Error
        }
        int in = rand() % 10;
        buffer.push_back(in);
        cout << "The Producer generated: " << in << endl;
        ReleaseMutex(Mutex); // Could be LeaveCriticalSection() instead
        ReleaseSemaphore(Full, 1, NULL); // 1 item added, update semaphore
    }
    cout << "\nThe PRODUCER produced " << nItems << " items." << endl;
    return 0; // OK
}

DWORD WINAPI consumer(LPVOID nCount)
{
    int nItems = (int)nCount;
    // Generate nCount numbers
    for (int i = 0; i < nItems; i++)
    {
        if (bExit) return 9; // Aborted
        WaitForSingleObject(Full, INFINITE); // Wait until at least one item in buffer
        // Lock the buffer and get an item
        WaitForSingleObject(Mutex, INFINITE); // Could be EnterCriticalSection() instead
        if (buffer.empty())
        {
            cout << "\nInternal Error: Buffer-empty Check Failed!" << endl;
            bExit = TRUE; // Tell all threads to exit
            ReleaseMutex(Mutex); 
            return 2; // Exit with Error
        }
        int out = buffer.front();
        buffer.erase(buffer.begin()); // Remove item from the list
        cout << "The Consumer ate: " << out << endl;
        ReleaseMutex(Mutex);  // Could be LeaveCriticalSection() instead
        ReleaseSemaphore(Empty, 1, NULL); // 1 item removed, update semaphore
    }
    cout << "\nThe CONSUMER consumed " << nItems << " items." << endl;
    return 0; // OK
}

Notes:

• The bExit variable is global and accessed/modified by more than one threads, but as this is always done within a critical section (mutex lock), there is no need for using another one, or interlocked variable access.
• The diagnostic messages (eg cout << "The Consumer ate: " << out << endl;), or any other kind of "processing" these data (or "working" on them) could have been placed after releasing the objects. This would be more efficient, releasing the objects earlier to the other thread(s). I've done it this way to better illustrate the sequence of events in your tests.
• If you set MAX_BUF to 1, numbers should be generated/printed one at a time, otherwise it can't be determined, but the items produced minus items consumed should of course never exceed the buffer size.