-1

I am using array with 2 threads. One is writing to it and another reading. You can assume that reading is slower than writing. The reader and writer are separate pthreads. As far as I know sharing an array as a global variable between those threads is safe. So overall picture is like: 

char ** array;

void writer(void){
    for (unsigned long i = 0; i < maxArraySize; i++){
     array[i] = malloc(someSize);
     array[i] = someCharArray;
}

void reader(void){
    for (unsigned long i = 0; i < maxArraySize; i++){
     if(array[i] == NULL){ // in case reader is faster
      i--;
      continue;
    }
     useData(array[i]);
    free(array[i]);      // HERE IS MY QUESTION..
}


main(){
    array  = malloc(maxArraySize);   
    pthread_t reader, writer;
    pthread_create( &reader, NULL, reader, NULL);
    pthread_create( &writer, NULL, writer, NULL);
}

My question is related with line where I free i'th element of array. Is it safe to do it? Because when I free i'th element, at the same time, write is writing to array. So can there be a case that writer gets wrong address as it can lose the head pointer?

Jeyhun Karimov
  • 1,295
  • 19
  • 27
  • 6
    Nothing you are doing is safe in any way. Look up what race conditions are. You have undefined behavior everywhere. – nwp Nov 02 '16 at 09:42
  • 1
    You need to protect all accesses to `array`. A mutex could be used for this. – Support Ukraine Nov 02 '16 at 09:44
  • @UnholySheep, I think c++ pops up in suggested tags when you post a question. Those who don't know any better think it's relevant to c. – StoryTeller - Unslander Monica Nov 02 '16 at 09:46
  • @nwp You mean it is not safe to share a global array between threads or to free an element in array? I got the answer for first argument here: http://stackoverflow.com/questions/5316379/can-threads-write-to-different-elements-of-same-array-of-structures-without-lock – Jeyhun Karimov Nov 02 '16 at 09:49
  • @JeyhunKarimov That question doesn't apply because your threads access the same elements, in that question they do not. [Read this](http://stackoverflow.com/a/34550/3484570). – nwp Nov 02 '16 at 09:52
  • @nwp In my case you can assume that writer is faster than reader and therefore they dont access the same elements. – Jeyhun Karimov Nov 02 '16 at 09:54
  • @nwp Moreover, while reading I check if array's i'th element is NULL, then it means that writer hasn't wrote yet. So, I try to pull again. But you can assume that this condition never happens. – Jeyhun Karimov Nov 02 '16 at 09:55
  • What is the writer supposed to do? He allocates an array with malloc and then assigns another array to array[i]? – k_kaz Nov 02 '16 at 09:57
  • 3
    *You can assume that reading is slower than writing.* No, you can't assume that. – Andrew Henle Nov 02 '16 at 09:58
  • @k_kaz yes. Basically `array` is char **. and writer assigns char * to every element of `array` – Jeyhun Karimov Nov 02 '16 at 09:59
  • 2
    If `array[i]` has not been written yet it contains garbage, not `NULL`, therefore your check is useless. It doesn't matter if the writer is faster, if you crank up optimization the compiler may assume that reader is an infinite loop, because `array` cannot change, because that would be undefined behavior. You can try to rely on undefined behavior and hope that it does exactly what you want, but you will be disappointed. Learn how to do multithreading properly and safe yourself a lot of time and broken code. – nwp Nov 02 '16 at 10:00
  • 1. In my usecase writer is always faster than reader. 2. I didn't use locks because of performance issues. So if I know that 2 threads will never modify the same element of an array, then no need to use locks IMO. – Jeyhun Karimov Nov 02 '16 at 10:07
  • @JeyhunKarimov *In my usecase writer is always faster than reader.* And how did you **measure** that? *I didn't use locks because of performance issues.* Did you **measure** the performance cost of any locking code? *So if I know that 2 threads will never modify the same element of an array, then no need to use locks IMO.* In the code you've provided, `reader()` can be complete and `free()` the entire array **before `writer()` even gets started.** – Andrew Henle Nov 02 '16 at 10:24
  • @AndrewHenle My question is: assuming writer is faster than reader always, is the code I provided is safe? In mu usecase it is always faster. – Jeyhun Karimov Nov 02 '16 at 10:29
  • @JeyhunKarimov *My question is: assuming writer is faster than reader always, is the code I provided is safe?* No, because you **CAN'T** assume that. How did you **measure** that the `writer()` is faster? How many times did you run your test? How do you even know that your `reader()` doesn't finish before `writer()` even gets started? The code you posted doesn't provide *any* of that information. You simply can not make that assumption. You might as well ask "Assuming the world is flat, what happens when I sail off the edge of the Earth?" – Andrew Henle Nov 02 '16 at 10:32
  • @AndrewHenle for example, reader is a socket and it is pull based and client pulls once an hour. So this is my usecase and I am sure that writer is faster. BTW, thanks for your examples :) – Jeyhun Karimov Nov 02 '16 at 10:37
  • @JeyhunKarimov *reader is a socket and it is pull based and client pulls once an hour. So this is my usecase and I am sure that writer is faster.* If it's that slow, you don't have any excuse for not using locking since performance is irrelevant. – Andrew Henle Nov 02 '16 at 11:29

1 Answers1

3

No it is not safe if you read during a write without a special instruction the result is undefined. You could get any value, though it is unlikely that you will see any other than NULL or the one you had assigned.

As others have mentioned in the comments the un-initialized array may contain anything (it is undefined) though it is likely zeroed before the kernel gave it to you.

If you want safety you need a locking mechanism such as a semaphore (http://man7.org/linux/man-pages/man3/sem_init.3.html). 

char ** array;
// Allows access while non zero
sem_t sem;

void writer(void){
    for (unsigned long i = 0; i < maxArraySize; i++){
     array[i] = malloc(someSize);
     array[i] = someCharArray;
     // Increment semaphore.
     sem_post(&sem);
}

void reader(void){
    for (unsigned long i = 0; i < maxArraySize; i++){
     // Will return -1 if the semaphore is not at zero
     // Will return 0 if semaphore is greater than zero and decrement it.
     if(sem_trywait(&sem)){ // in case reader is faster
      i--;
      continue;
    }
    useData(array[i]);
    free(array[i]);      // HERE IS MY QUESTION..
}


main(){
    // Initialize semaphore to zero
    sem_init(&sem, 0 , 0);
    // Initialize array to have maxArraySize elements.
    array  = malloc(maxArraySize * sizeof(*array));   
    pthread_t reader, writer;
    pthread_create( &reader, NULL, reader, NULL);
    pthread_create( &writer, NULL, writer, NULL);
}

This should be fast but will spin your cpu doing a lot of nothing at the sem_trywait. Use sem_wait if you can wait a little longer and do not need the spinning.

I also corrected the bug in your malloc statement because it was not allocating enough space for maxArraySize char * items.

Trevor Barnwell
  • 46
  • 4
  • Thanks for answer. Is is safe in your implementation, to free `array[i]` after reading it? Can this cause some trouble to writer? – Jeyhun Karimov Nov 02 '16 at 10:53
  • 1
    You're welcome. Yes it is thread safe to free array[i]. Of course no other thread can be using it. This should not cause trouble for the writer because malloc and free are thread safe. (http://man7.org/linux/man-pages/man3/malloc.3.html, attributes section). – Trevor Barnwell Nov 02 '16 at 11:02
  • 1
    Thanks for the tip. I realized I messed up (a few minutes ago) but can't edit the comment now. – Trevor Barnwell Nov 02 '16 at 11:18