Thread sync using conditional variables

Question

Can you review the code?

#include <stdio.h>
#include <pthread.h>
#include <windows.h>

#include "bin/include/pthread.h"
#include "bin/include/sched.h"
#include "bin/include/semaphore.h"


#define TCOUNT 10
#define WATCH_COUNT 12

int count = 0;
pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t count_threshold_cv = PTHREAD_COND_INITIALIZER;
int  thread_ids[2] = {1,2};

/* Function prototypes */
void watch_count(int *idp);
void inc_count(int *idp);
int getDirFileCount(void);
/* End of fuction prototypes */


int main(void)
{
    pthread_t threads[2];
    int retCode = 0;
    int initialFileCount = getDirFileCount();
    count = initialFileCount;
    printf("No of Files initially : %d \n",initialFileCount);
    if ((retCode = pthread_create(&threads[0],NULL,(void *)&inc_count, &thread_ids[0]))) {
        perror("Thread creation error : can't able to create thread");
    }
    if ((retCode = pthread_create(&threads[1],NULL,(void *)&watch_count, &thread_ids[1]))) {
        perror("Thread creation error : can't able to create thread");
    }

    pthread_exit(NULL);
    return 0;
}

void watch_count(int *idp)
{
    pthread_mutex_lock(&count_mutex);
    //int watchCount = getDirFileCount();
    while (1) {
        printf("waiting for changes made to the directory \n");
        pthread_cond_wait(&count_threshold_cv,
                          &count_mutex);
        printf("OK , now someone made chaneges to the directory\n");
    }
    pthread_mutex_unlock(&count_mutex);
}
void inc_count(int *idp)
{
    for (;;) {
        pthread_mutex_lock(&count_mutex);
        int countnow = getDirFileCount();
        printf("Number of files now: %d \n",countnow );
        if (count < countnow) {
            pthread_cond_signal(&count_threshold_cv);
            pthread_mutex_unlock(&count_mutex);
            return;
        }
        //pthread_cond_signal(&count_threshold_cv);
        pthread_mutex_unlock(&count_mutex);
        //pthread_exit(NULL)

    }
}


int getDirFileCount(void)
{
    int count = 0;
    WIN32_FIND_DATA fd;
    char *dirFilePath = "D:\\Test\\*";
    HANDLE h = FindFirstFile(dirFilePath, &fd);
    if (h != INVALID_HANDLE_VALUE) {
        do {
            count++;
        } while (FindNextFile(h, &fd));
        FindClose(h);
    }
    return count;
}

Answer 1

Could be simplified

For what you are doing, using a mutex and a condition variable is more complex than necessary. You could just use a semaphore or signal instead. A mutex should guard some shared variable or state, but your code doesn't have any. Instead, you are using the mutex + condition variable simply to wake up another thread.

As far as your implementation goes, I have the following comments:

• A loop containing pthread_cond_wait() should always check a predicate variable to guard against spurious wakeups. This is in the documentation for pthread_cond_wait(). So for example:

  pthread_mutex_lock(&count_mutex);
  while (count == initialCount) {
      pthread_cond_wait(&count_threshold_cv, &count_mutex);
  }
  // Do something here...
  pthread_mutex_unlock(&count_mutex);
  

• The loop doing the work should lock the mutex for as short a time as possible. You only need to hold the mutex when you want to access shared variables. So for example:

  for (;;) {
      // Do this long work outside of the mutex
      int countnow = getDirFileCount();
  
      if (countnow > initialCount) {
          // Only lock the mutex when you need to actually need to access
          // shared state (in this case the variable "count").
          pthread_mutex_lock(&count_mutex);
          count = countnow;
          pthread_cond_signal(&count_threshold_cv);
          pthread_mutex_unlock(&count_mutex);
          return;
      }
  }