0

I need to create a thread for each incoming person. It works fine creating a thread for each person. But only the last thread lives. the other ones are somehow overwritten by the last one. what am I doing wrong here? Do you see anything wrong with this code?

  Person *person; 
    DWORD bytesRead;

    while(1)
    {   

        person = (Person *) malloc( sizeof(Person) );
        bytesRead = mailslotRead ( mailbox, &person, sizeof(Person) ); 
        if( bytesRead != 0 )
        {
            list_append( planets, &person );
            threadCreate(personThread, &person );
            Sleep(100);
        }
    }


DWORD WINAPI personThread(Person* person)
{

    MessageBox(0, person->name, "New thread created for:", 0);
    while(1)
    {
        person->age += 1;
        MessageBox(0, person->name, "person aged", 0);
        Sleep(5000);
    }

    MessageBox(0, person->name, "Thread died", 0);
}

1 Answers1

1

Don't pass &person to the thread, pass person. The variable person will be overwritten, and since the threads are accessing the location where person lives, they will all access the same data. You really want to pass the value of person, i.e. the newly allocated memory address, to each newly created thread.

And please don't cast the return value of malloc() in C.

UPDATE: The code you're showing for personThread() fails to treat its argument properly (it should be void *, cast to Person ** and dereferenced) so I think you're also invoking undefined behavior.

Community
  • 1
  • 1
unwind
  • 391,730
  • 64
  • 469
  • 606
  • If the OP willes pass `person` to the thread function it shall cast its argument to `Person *`. – alk Feb 19 '14 at 14:05
  • @alk Please read the `malloc()` link for my thoughts about casting pointers to/from `void *` in C. – unwind Feb 19 '14 at 14:13
  • I was referring to the cast necessary inside the thread function. – alk Feb 19 '14 at 14:15