Making threads fails after a while

Question

I wrote a code which uses threads in a Linux C++ program. But it fails after a while, I don't know why. I think there may be a memory leakage somewhere. This is a simplified version:

#include <stdlib.h>
#include <iostream>
#include <stdio.h>
#include <pthread.h>
#include <signal.h>

using namespace std;


#define MAX_THREADS  20
#define THREAD_STACK  100000
pthread_t pid[MAX_THREADS];

unsigned thread_args[MAX_THREADS][2];
volatile unsigned thread_number = 0;

void* TaskCode(void* parg)
{
    unsigned a = ((unsigned *)parg)[0];
    unsigned b = ((unsigned *)parg)[1];

    for(int i = 0; i < 1000; i++)
        ;
    cout<< "\n\n" << a << "  " << b << "\n\n";

    thread_number--;
    return 0;
}

void Action(unsigned long a,unsigned b)
{
    if(thread_number >= MAX_THREADS)
        return;
    pthread_attr_t  attrs;
    pthread_attr_init(&attrs);
    pthread_attr_setstacksize(&attrs, THREAD_STACK);
    thread_args[thread_number][0] = a;
    thread_args[thread_number][1] = b;
    if(pthread_create(&pid[thread_number],&attrs, TaskCode, (void*) thread_args[thread_number]) != 0)
    {
        cout<< "\n\n" "new thread failed. thread number:" << thread_number << "\n\n";
        for(unsigned i = 0; i < thread_number; i++)
            pthread_kill(pid[i], SIGSTOP);
    }
    thread_number++;
}

int main()
{
    int a = 0;
    while(true)
    {
        for(int i = 0; i < 1000; i++)
            ;
        Action(time(0),1);
    }

    cout<< "\n\nunexpected end\n\n";
}

What's wrong with it?

Edit: As suggested I changed the code:

#include <stdlib.h>
#include <iostream>
#include <stdio.h>
#include <pthread.h>
#include <signal.h>

using namespace std;

#define MAX_THREADS  20
#define THREAD_STACK  100000
pthread_t pid[MAX_THREADS];

unsigned thread_args[MAX_THREADS][2];
volatile unsigned thread_number = 0;

pthread_mutex_t mutex_;

void* TaskCode(void* parg)
{
    unsigned a = ((unsigned *)parg)[0];
    unsigned b = ((unsigned *)parg)[1];

    for(int i = 0; i < 1000; i++)
        ;
    cout<< "\n\n" << a << "  " << b << "\n\n";
    pthread_mutex_lock(&mutex_);
    thread_number--;
    pthread_mutex_unlock(&mutex_);
    return 0;
}

void Action(unsigned long a,unsigned b)
{
    if(thread_number >= MAX_THREADS)
        return;
    pthread_attr_t  attrs;
    pthread_attr_init(&attrs);
    pthread_attr_setstacksize(&attrs, THREAD_STACK);
    thread_args[thread_number][0] = a;
    thread_args[thread_number][1] = b;
    if(pthread_create(&pid[thread_number],&attrs, TaskCode, (void*) thread_args[thread_number]) != 0)
    {
        cout<< "\n\n" "new thread failed. thread number:" << thread_number << "\n\n";
        for(unsigned i = 0; i < thread_number; i++)
            pthread_kill(pid[i], SIGSTOP);
    }
    pthread_mutex_lock(&mutex_);
    thread_number++;
    pthread_mutex_unlock(&mutex_);
}

int main()
{
    int a = 0;
    pthread_mutex_init(&mutex_, NULL);
    while(true)
    {
        for(int i = 0; i < 1000; i++)
            ;
        Action(time(0),1);
    }
    pthread_mutex_destroy(&mutex_);
    cout<< "\n\nunexpected endn\n";
}

Still it fails.

Where exactly does it fail? What's the error message do you get? — brokenfoot, Apr 01 '14 at 22:28
just `pthread_create` keeps returning nonzero after a few seconds; for the code above. — Minimus Heximus, Apr 01 '14 at 22:31
interesting how you always reuse the low-numbered thread slots early. — Deduplicator, Apr 01 '14 at 22:40
Any reason you give each thread 100000Byte stack address space? 4KB should suffice, not that it really matters... — Deduplicator, Apr 01 '14 at 22:43
@Deduplicator: just to be sure it suffices.bt the changing it will not prevent failure. — Minimus Heximus, Apr 01 '14 at 22:46
Your main thread is not cleaning up after the threads are killed. If the threads are joinable, join them so their resources can be cleaned up. Alternatively, create detached threads if possible so automatic cleanup will occur. — alvits, Apr 01 '14 at 23:23
@MinimusHeximus - `pthread_attr_setdetachstate(&attrs, PTHREAD_CREATE_DETACHED)` will set the attrs appropriately. Put this line after the `pthread_init(&attrs)`. — alvits, Apr 01 '14 at 23:40
@alvits: thanks. It seems to be a solution. if that was not a comment I could accept it as an answer. — Minimus Heximus, Apr 01 '14 at 23:45

score 2 · Answer 1 · answered Apr 01 '14 at 23:10

Your program fails in one of two ways for me.

You run out of memory.
thread_number gets corrupted due to locking as noted by @acarlon.

If the program runs long enough (thread_number isn't corrupted), then you run out of memory due to spawning of threads. You spawn threads very quickly. You never join the threads after they exit. Each thread continues to sit in memory until pthread_join is called so that you can recover it's return code.

To verify, print errno when pthread_create fails:

cout << "pthread_create failed:" << strerror(errno) << endl;

The above prints (if it executes long enough):

pthread_create failed: Cannot allocate memory

You will need to include string.h and errno.h.

The second failure case I noted is when thread_number gets corrupted. You can verify this by printing thread_number in your TaskCode. You will note that you sometimes get values outside of 0-20 (4294967295 for example). This is the corruption noted by @acarlon.

if I add `pthread_join(pid[this_thread_number],0);` after calling `pthread_create`, making new threads working separately, becomes pointless! — Minimus Heximus, Apr 01 '14 at 23:34

score 2 · Accepted Answer · answered Apr 01 '14 at 23:54

By default threads are created with detachstate set to joinable. This requires that threads be joined to get the return code of a thread and cleanup the resources used.

Without cleaning up you'll run out of memory really quickly (memory leak).

For cases where threads need not return any code and cleanup of memory can be done as soon as the thread exits, then detached threads can be created.

To set the attribute such that all threads created will be detached use pthread_attr_setdetachstate(). Set the value to PTHREAD_CREATE_DETACHED.

pthread_init(&attrs);
pthread_attr_setcreatedetached(&attrs,PTHREAD_CREATE_DETACHED);

score 1 · Answer 3 · edited May 23 '17 at 11:49

1

 volatile unsigned thread_number = 0;

Volatile on its own isn't safe you need to use interlocked increment and decrement or use a mutex around the thread_number. Volatile will guarantee that threads do not operate on local cached copies. So, if you write a value to a shared variable on one thread, the other threads will see the new value when they read it. However, it does not guarantee that operations occur atomically.

See this answer which is actually for C#, but has the right idea.

edited May 23 '17 at 11:49

Community

1
1

answered Apr 01 '14 at 22:32

acarlon

16,764
7
75
94

I changed my code (see my question), I'm not sure if this is exactly what you suggested. – Minimus Heximus Apr 01 '14 at 22:55
@MinimusHeximus - Your new code looks fine regarding the locks. There was also the problem or running out of memory that esorton found. – acarlon Apr 01 '14 at 23:26

Minimus Heximus · Answer 4 · 2014-04-03T09:39:56.217

Thanks to all answers and comments this is my final code, I add to archive:

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

using namespace std;


#define MAX_THREADS  20
#define THREAD_STACK  100000
pthread_t pid[MAX_THREADS];


unsigned thread_args[MAX_THREADS][3];
volatile bool pid_flags[MAX_THREADS] = {0};// all false

pthread_mutex_t mutex_;

void* TaskCode(void* parg)
{
    unsigned a = ((unsigned *)parg)[0];
    unsigned b = ((unsigned *)parg)[1];
    unsigned index = ((unsigned*)parg)[2];

    for(int i = 0; i < 1000; i++)
        ;
    cout<< "\n\n" << a << "  " << b << "\n\n";


    pthread_mutex_lock(&mutex_);
    pid_flags[index] = false;
    pthread_mutex_unlock(&mutex_);
    return 0;
}

void Sleep(unsigned milliseconds)
{
    usleep(milliseconds * 1000);
}

bool FreeIndex(unsigned& index)
{
    bool b;
    for(index = 0; index < MAX_THREADS; index++)
    {
        pthread_mutex_lock(&mutex_);
        b = pid_flags[index];
        pthread_mutex_unlock(&mutex_);
        if(b == false)
            return true;
    }
    return false;
}

void Action(unsigned long a,unsigned b)
{
    pthread_attr_t  attrs;
    pthread_attr_init(&attrs);
    pthread_attr_setdetachstate(&attrs,PTHREAD_CREATE_DETACHED);
    pthread_attr_setstacksize(&attrs, THREAD_STACK);


    unsigned free_index;

    while(!FreeIndex(free_index))
        Sleep(50);

    thread_args[free_index][0] = a;
    thread_args[free_index][1] = b;
    thread_args[free_index][2] = free_index;
    if(pthread_create(&pid[free_index],&attrs, TaskCode, (void*) thread_args[free_index]) != 0)
    {
        cout<< "\n\n" "Threading failed." "\n\n";
        bool b;
        for(unsigned i = 0; i < MAX_THREADS; i++)
        {
            pthread_mutex_lock(&mutex_);
            b = pid_flags[i];
            pthread_mutex_unlock(&mutex_);
            if(b == true)
                pthread_kill(pid[i], SIGSTOP);
        }
    }
    pthread_mutex_lock(&mutex_);
    pid_flags[free_index] = true;
    pthread_mutex_unlock(&mutex_);
}

int main()
{
    pthread_mutex_init(&mutex_,0);
    while(true)
    {
        for(int i = 0; i < 1000; i++)
            ;
        Action(time(0),1);
    }
    pthread_mutex_destroy(&mutex_);
    cout<< "\n\n" "unexpected end" "\n\n";
}

Any improvement comments is appreciated.

Making threads fails after a while

4 Answers4

Linked