views:

232

answers:

1

Hello, i have the following code:

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

#define NUM_THREADS 100

struct thread_param {
    char *f1;
    char *f2;
    int x;
};

void *thread_function(void *arg){
    printf("%d\n", ((struct thread_param*)arg)->x);
}

int main(int argc, char *argvs[]){
    int i, thread_cr_res = 0, thread_join_res;
    pthread_t *threads;
    threads = malloc(100 * sizeof(*threads));
    if(threads == NULL){
        fprintf(stderr,"MALLOC THREADS ERROR");
        return (-1);
    }
    for(i = 0; i < NUM_THREADS; i++){
        struct thread_param *tp;
        if((tp = malloc(sizeof(*tp))) == NULL){
            fprintf(stderr,"MALLOC THREAD_PARAM ERROR");
            return (-1);
        }
        tp->f1 = "f1";
        tp->f2 = "f2";
        tp->x = i;
        thread_cr_res = pthread_create(&threads[i], 
                    NULL, 
                    thread_function, 
                    (void*)tp);
        if(thread_cr_res != 0){
            fprintf(stderr,"THREAD CREATE ERROR");
            return (-1);
        }
    }
    return (0);
}

What i want to achieve, is to print all the numbers from 0 to 99, from threads. Also i am experimenting a way to pass a structure as a thread input parameter.

What i am finding curios, is that not all the numbers are shown, eg:

 ./a.out | grep 9
9
19
29
39
49

And sometimes some numbers are shown twice:

...
75
74
89
77
78
79
91
91

Can you please explain me why is this happening ? No errors are shown.

LATER EDIT: I've rewritten the code as Yasir suggested, using a pthread_join. The new code looks like this:

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

#define NUM_THREADS 100

struct thread_param {
    char *f1;
    char *f2;
    int x;
};

void *thread_function(void *arg){
    printf("%d\n", ((struct thread_param*)arg)->x);
}

int main(int argc, char *argvs[]){
    int i, thread_cr_res = 0, thread_join_res;
    pthread_t *threads;
    threads = malloc(100 * sizeof(*threads));
    if(threads == NULL){
        fprintf(stderr,"MALLOC THREADS ERROR");
        return (-1);
    }
    for(i = 0; i < NUM_THREADS; i++){
        struct thread_param *tp;
        if((tp = malloc(sizeof(*tp))) == NULL){
            fprintf(stderr,"MALLOC THREAD_PARAM ERROR");
            return (-1);
        }
        tp->f1 = "f1";
        tp->f2 = "f2";
        tp->x = i;
        thread_cr_res = pthread_create(&threads[i], 
                    NULL, 
                    thread_function, 
                    (void*)tp);
        if(thread_cr_res != 0){
            fprintf(stderr,"THREAD CREATE ERROR");
            return (-1);
        }
    }
    /* Later edit, joining the threads */
    for (i = 0; i < NUM_THREADS; i++){
        thread_join_res = pthread_join(threads[i], NULL);
        if(thread_join_res != 0){
            fprintf(stderr, "JOIN ERROR");
            return (-1);
        }       
    }
    return (0);
}

After:

./a.out | sort
0
1
10
11
12
13
14
15
16
17
18
19
2
20
21
22
23
24
25
26
27
28
29
3
30
31
32
33
34
35
36
37
38
39
4
40
41
42
43
44
45
46
47
48
49
5
50
51
52
53
54
55
56
57
58
59
6
60
61
62
63
64
65
66
67
68
69
7
70
71
72
73
74
75
76
77
78
79
8
80
81
82
83
84
85
86
87
88
89
9
90
91
92
93
94
95
96
97
98
99

The code is acting like it should. Still i cannot explain myself why the first version of the code was outputting duplicates...

+1  A: 

Use int pthread_join(pthread_t thread, void **value_ptr) to wait for threads termination in order to get all results before main thread exit. Also because of sizeof(*tp) you end up with size of pointer to this struct which is 4 bytes long on 32 bit system. This could possibly rewrite other structures in memory. sizeof(thread_param) would make more sense to me. Also tp->f1 = "f1"; refers to one constant string. I. e. you don't save a string in structure but rather used the same buffer for all your thread_param structures. This would be unsafe if "f1" is a pointer to variable buffer.

UPD: Yes, comments bellow about size are correct.

Yasir Arsanukaev
This may resolve the issue regarding the lines that aren't showing at all. Still, why some numbers are shown more than once ?
Andrei Ciobanu
Maybe because of `sizeof(*tp)` ? I suppose `sizeof(thread_param)` would make more sense, otherwise we end up with size of pointer to this struct which is 4 bytes long on 32 bit system.
Yasir Arsanukaev
sizeof(*tp) is the same as sizeof(thread_param) . Anyways thanks for the joining tip.
Andrei Ciobanu
@Yasir: What's wrong with `sizeof(*tp)`?
KennyTM
@Yasir sizeof(*tp) means "the size of the thing that tp is a pointer to"
anon