Why does pthread_join fail on the last iteration (giving segmentation fault)?

Question

Multi-threading beginner here. On exactly the 5th iteration (i.e. when executing pthread_join(threadID[4], NULL), my program fails with a segmentation fault.

I am creating multiple threads to add/subtract 1 from a counter variable to study race conditions. Everything works smoothly until I try 5 threads or more. On exactly the last iteration of pthread_join(threadID[4], NULL), it fails and I can't determine why. I am sure the issue is there as I used printf statements to see where it reaches before failing.

#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>
#include <pthread.h>
#include <stdint.h>
#include <errno.h>
#include <string.h>
#include <getopt.h>
#include <time.h>

int opt_threads;
int opt_iterations;
long nThreads;
long nIterations;
int opt_yield;
long long counter;

void add(long long *pointer, long long value) {
  long long sum = *pointer + value;
  if (opt_yield)
    sched_yield();
  *pointer = sum;
}

void *thread_worker(void * arg) {
  long i;
  for (i=0; i<nIterations; i++) {
    add(&counter, 1);
    add(&counter, -1);
  }

  return arg;
}

int main(int argc, char *argv[]) {
  int c;

  pthread_t *threadID = malloc(nThreads * sizeof(pthread_t));
  if (threadID == NULL) {
    fprintf(stderr, "Thread memory allocation failed\n");
    exit(1);
  }

  static struct option long_options[] =
    {
      {"threads", required_argument, 0, 't'},
      {"iterations", required_argument, 0, 'i'},
      {"yield", no_argument, 0, 'y'},
      {0,0,0,0}
    };

  while (1) {
    c = getopt_long(argc, argv, "", long_options, NULL);
    if (c==-1) break;

    switch(c) {
    case 't':
      opt_threads = 1;
      nThreads = atoi(optarg);
      break;
    case 'i':
      opt_iterations = 1;
      nIterations = atoi(optarg);
      break;

    case 'y':
      opt_yield = 1;
      break;

    default:
      fprintf(stderr, "Bad argument!\n");
      exit(1);
    }
   }

counter = 0;
struct timespec start, finish;
int i;

//start clock
clock_gettime(CLOCK_MONOTONIC, &start);

//create
for (i=0; i < nThreads; i++) {
  pthread_create(&threadID[i], NULL, &thread_worker, NULL);
  printf("Created thread[%ld]\n", i);
}

//wait (join)
  /*for (i=0; i < nThreads; i++) {
    printf("Now i is %ld\n", i);
    if (pthread_join(threadID[i], NULL) != 0)
      fprintf(stdout,"ERRRRROOOORRRRRRRR\n");

  }*/

  pthread_join(threadID[0], NULL);
  pthread_join(threadID[1], NULL);
  pthread_join(threadID[2], NULL);
  pthread_join(threadID[3], NULL);
  pthread_join(threadID[4], NULL);

  printf("about to end clock\n");
  //finish clock
  clock_gettime(CLOCK_MONOTONIC, &finish);

  printf("finished clock\n");

  long seconds = finish.tv_sec - start.tv_sec;
  long ns = finish.tv_nsec - start.tv_nsec;
  long runTime = (seconds + ns) * 1000000000L;
  long nOperations = nThreads * nIterations * 2;
  long avgOperations = runTime / nOperations;
  long run_time = 1000000000L * (finish.tv_sec - start.tv_sec) + finish.tv_nsec - start.tv_nsec;

  //Print
  if (opt_yield == 0)
    fprintf(stdout, "add-none, %ld, %ld, %lld, %ld, %lld, %lld\n", nThreads, nIterations, nOperations, run_time, run_time/nOperations, counter);

  else if (opt_yield == 1)
    fprintf(stdout, "add-yield-none, %ld, %ld, %lld, %ld, %lld, %lld\n",nThreads, nIterations, nOperations, run_time, run_time/nOperations, counter);

exit(0);
}

I expect the program to be able to correctly wait for the 5th thread, but it fails with segmentation fault.

Answer 1

Your main function starts:

int main(int argc, char *argv[])
{
    int c;

    pthread_t *threadID = malloc(nThreads * sizeof(pthread_t));

Since nThreads is a global variable without an explicit initializer, its value is zero — you haven't allocated any memory that you can legitimately use. Accessing that memory leads to undefined behaviour.

Defer that memory allocation until after you know how many threads you'll need.

Undefined behaviour means anything can happen, including appearing to work until it doesn't work any more.

You also need to rewrite the thread joining code as a loop to match the loop that creates the threads.

Answer 2

There are a LOT of problems with the posted code.

Rather than listing them all, I'll just present a working version of the code. You can perform a comparison to expose the changes.

Note: I did provide some default parameter values when no command line parameters are provided by the user. Otherwise the divide by nIterations results in a 'divide by 0' exception

I did make use of the Variable Length Array feature of C to declare an array of pThread_t rather than use dynamic memory allocation.

In general, it is a poor programming practice to include header files those contents are not used

Note that in function: add() that 1) the common counter value update is not being protected by a pthread_mutex_t so the count can/will be corrupted by the other threads. Also calling sched_yield() between updating the local variable sum and the updating of the global counter means the results of the other threads can/will be obliterated.

#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
//#include <unistd.h>
#include <pthread.h>
//#include <stdint.h>
//#include <errno.h>
//#include <string.h>
#include <getopt.h>
#include <time.h>

int opt_threads;
int opt_iterations;
long nThreads;
long nIterations;
int opt_yield;
long long counter;

void add(long long *pointer, long long value) 
{
    long long sum = *pointer + value;
    if (opt_yield)
    {
        sched_yield();
    }
    *pointer = sum;
}

void *thread_worker(void * arg) 
{
    void(arg);
    //long i;
    for ( long i=0; i<nIterations; i++) 
    {
        add(&counter, 1);
        add(&counter, -1);
    }

    //return arg;
    pthread_exit( NULL );
}

int main(int argc, char *argv[]) 
{
    int c;

    static struct option long_options[] =
    {
        {"threads", required_argument, 0, 't'},
        {"iterations", required_argument, 0, 'i'},
        {"yield", no_argument, 0, 'y'},
        {0,0,0,0}
    };

    while ( (c = getopt_long(argc, argv, "", long_options, NULL)) != -1 )
    {
        switch(c) 
        {
            case 't':
                opt_threads = 1;
                nThreads = atoi(optarg);
                break;

            case 'i':
                opt_iterations = 1;
                nIterations = atoi(optarg);
                break;

            case 'y':
                opt_yield = 1;
                break;

            default:
                fprintf(stderr, "Bad argument!\n");
                exit( EXIT_FAILURE );
                break;
        }
    }

    if( nIterations == 0 )
    {
        nIterations = 1;
        opt_iterations = 1;
    }

    if( nThreads == 0 )
    {
        nThreads = 5;
        opt_threads = 1;
    }

    pthread_t threadID[ nThreads ];

    counter = 0;
    struct timespec start, finish;
    //int i;

    //start clock
    clock_gettime(CLOCK_MONOTONIC, &start);

    //create
    for ( int i=0; i < nThreads; i++) 
    {
        if( pthread_create(&threadID[i], NULL, &thread_worker, NULL) < 0 )
        {
            perror( "pthread_create failed" );
            exit( EXIT_FAILURE );
        }
        printf("Created thread[%d]\n", i);
    }

    for( int i = 0; i<nThreads; i++ )
    {
        pthread_join(threadID[i], NULL);
    }

    printf("about to end clock\n");
    //finish clock
    clock_gettime(CLOCK_MONOTONIC, &finish);

    printf("finished clock\n");

    //long seconds = finish.tv_sec - start.tv_sec;
    //long ns = finish.tv_nsec - start.tv_nsec;
    //long run_Time = seconds * 1000000000L + ns;
    long nOperations = nThreads * nIterations * 2;
    //long avgOperations = run_Time / nOperations;
    long run_duration = 1000000000L * (finish.tv_sec - start.tv_sec) + finish.tv_nsec - start.tv_nsec;

    //Print
    if (opt_yield == 0)
    fprintf(stdout, "add-none, %ld, %ld, %ld, %ld, %ld, %lld\n", 
        nThreads, 
        nIterations, 
        nOperations, 
        run_duration, 
        run_duration/nOperations, 
        counter);

    else 
    {
        fprintf(stdout, "add-yield-none, %ld, %ld, %ld, %ld, %ld, %lld\n",
            nThreads, 
            nIterations, 
            nOperations, 
            run_duration, 
            run_duration/nOperations, 
            counter);
    }

    exit(0);
}

The above code, with no command line parameters results in the following output.

Created thread[0]
Created thread[1]
Created thread[2]
Created thread[3]
Created thread[4]
about to end clock
finished clock
add-none, 5, 1, 10, 3544641, 354464, 0