CODEWITHSUNDEEP
clinuxmultithreadingpthreads
Brendan Honea
Brendan Honea

Reputation: 19

Odd behavior when creating many threads using a for loop

I'm in the process of making a multi threaded program that simulates one elevator and 49 people, with each person having their own thread, as well as the elevator. I'm in the beginning stages and just trying to get the threads created and verifying that they work properly.

I have an array of pthreads called person_threads that I am trying to initialize in a for loop, within the loop I am sending a message with the thread number and printing it out within the thread. For some reason I am getting weird behavior, almost like the for loop is not properly iterating before creating some threads (see loop and output). This behavior is random and various with each run, and I am not sure what I need to do to get the threads to correctly create. Please help if you have any insight on what might be causing this issue.

Code

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <unistd.h>

#define MAX_PERSONS 49

void *person(void *myvar);
void *elevator(void *myvar);

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

    pthread_t elevator_thread;
    pthread_t person_threads[MAX_PERSONS]; //My array of threads

    char *elev_msg = "Elevator thread started";

    pthread_create(&elevator_thread, NULL, elevator, (void *) elev_msg);

    //Here is where I try to initialize messages and threads,
    //see Persons function and output    

    for (int i = 0; i < MAX_PERSONS; i++){
        char msg[50];
        snprintf(msg, sizeof msg, "Person thread %d started", i);
        pthread_create(&person_threads[i], NULL, person, (void *) msg);
    }

    printf("Main function after pthread_creates\n");

    pthread_join(elevator_thread, NULL);
    for (int i = 0; i < MAX_PERSONS; i++){
        pthread_join(person_threads[i], NULL);
    }

    return 0;
}

void *person(void *myvar){
    char *msg;
    msg = (char *) myvar;

    printf("%s\n", msg);

    return NULL;
}

void *elevator(void *myvar){
    char *msg;
    msg = (char *) myvar;

    printf("%s\n", msg);

    return NULL;
}

Output

brendan@brendan-Ubuntu-Desk:~/Documents/OS-Project2$ ./elevator 
Elevator thread started
Person thread 7 started
Person thread 7 started
Person thread 7 started
Person thread 7 started
Person thread 7 started
Person thread 7 started
Person thread 7 started
Person thread 8 started
Person thread 9 started
Person thread 10 started
Person thread 12 started
Person thread 12 started
Person thread 14 started
Person thread 15 started
Person thread 15 started
Person thread 16 started
Person thread 17 started
Person thread 18 started
Person thread 19 started
Person thread 20 started
Person thread 21 started
Person thread 22 started
Person thread 23 started
Person thread 24 started
Person thread 25 started
Person thread 26 started
Person thread 27 started
Person thread 28 started
Person thread 29 started
Person thread 30 started
Person thread 31 started
Person thread 32 started
Person thread 33 started
Person thread 34 started
Person thread 35 started
Person thread 36 started
Person thread 37 started
Person thread 38 started
Person thread 39 started
Person thread 40 started
Person thread 41 started
Person thread 42 started
Person thread 43 started
Person thread 44 started
Person thread 45 started
Person thread 46 started
Person thread 47 started
Person thread 48 started
Main function after pthread_creates
Person thread 48 started

Notice how there are multiple 7s, 15s, 12s, and 48s. This behavior is random with each run. I have noticed that it does in fact always create 50 threads but I need the array to be initialized correctly. Any help would be appreciated.

Upvotes: 0

Views: 173

Answers (3)

alk
alk

Reputation: 70893

As others mentioned msg's memory gets deallocated the moment the loop starts over (or ends).

Besides allocating on the heap you could just define an array of "string"s like this:

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

  pthread_t elevator_thread;
  pthread_t person_threads[MAX_PERSONS]; //My array of threads
  char msg[MAX_MSG_LEN + 1][MAX_PERSONS];
  char *elev_msg = "Elevator thread started";

  pthread_create(&elevator_thread, NULL, elevator, (void *) elev_msg);

  for (size_t i = 0; i < MAX_PERSONS; i++){
    snprintf(msg[i], sizeof msg, "Person thread %zu started", i);
    pthread_create(&person_threads[i], NULL, person, msg[i]);
  }

  ...

or even better bring together what belongs together:

struct Person_Thread 
{
  pthread_t thread;
  char msg[MAX_MSG_LEN + 1];
};

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

  pthread_t elevator_thread;
  struct Person_Thread person_threads[MAX_PERSONS]; //My array of threads
  char *elev_msg = "Elevator thread started";

  pthread_create(&elevator_thread, NULL, elevator, (void *) elev_msg);

  for (size_t i = 0; i < MAX_PERSONS; i++){
    snprintf(person_threads[i].msg, sizeof person_threads[i].msg, 
      "Person thread %zu started", i);

    pthread_create(&person_threads[i].thread, NULL, person, 
      person_threads[i].msg]);
  }

  ...

Upvotes: 0

Carey Gregory
Carey Gregory

Reputation: 6846

Your mistake is here:

  for (int i = 0; i < MAX_PERSONS; i++){
    char msg[50];

Since you're using a local variable for the message buffer, on each iteration of the loop that buffer is getting created on the stack and then destroyed. And then on the next iteration you're likely reusing the same memory and overwriting it.

You need to allocate msg dynamically using malloc for each thread so they all have their own message buffer that isn't being shared and getting overwritten.

Upvotes: 1

Zbynek Vyskovsky - kvr000
Zbynek Vyskovsky - kvr000

Reputation: 18825

Here is the cause:

for (int i = 0; i < MAX_PERSONS; i++){
    char msg[50];
    snprintf(msg, sizeof msg, "Person thread %d started", i);
    pthread_create(&person_threads[i], NULL, person, (void *) msg);
}

The msg parameter is valid only in the main thread within the scope of the block. You need to allocate the msg buffer on the heap and free it in the child thread.

Upvotes: 2

Related Questions