Weird issue with returning empty struct instead of NULL

Question

Context

I am working on a consumer & producer multi-threaded program. The program has a shared variable num_elem that is incremented in the producer and decremented in the consumer.

num elem is providing information for a circular buffer that is using the struct queue, such that queue stores arrays of words being read from a file.

If the increment is changed from being inside of producer to being a function called by producer, then communication between the producer and consumer is fixed. Else, if the code to producer called as a function (as consumer is), then the used boolean is changed to TRUE, when it is in fact not being used.

The code for each is below.

Problem

If the increment is called outside of producer, then the values to used are changed to true. Why are they being changed when being called inside of producer, and not the when called as a function that is inside of producer?

CODE for producer

item_t *item = NULL;
for (i = 0; i < QUEUE_SIZE; i++) {
    if (queue[(next_index + i) % QUEUE_SIZE].used == false) {
        item = &queue[(next_index + i) % QUEUE_SIZE];
        item->used = true;
        num_elem++;
        next_index = (i + 1) % QUEUE_SIZE;
    }
}

CODE for consumer

item_t *consume_item() {
    for (i = 0; i < QUEUE_SIZE; i++) {
        if (queue[i].used == true) {
            item_t *item = &queue[i];
            item->used = false;
            num_elem--;
            return item;
        }
    }
    return NULL;

Here is the github repo, if you would like to see the whole code

How are you making sure that both threads are not modifying `num_elem` at the same time? — R Sahu, Oct 31 '14 at 05:22
@RSahu , consumer and producer both have their own while loops, with a mutex locking and unlocking the buffer at the beginning and end of each producer and consumer. There are also condition variables and a boolean for the while loop. I hope this answers your question, because I didn't want to throw up the entire code :) — T.Woody, Oct 31 '14 at 05:28
Here is the [github repo](https://github.com/Twoody/wordFreq-with-producer-consumer/tree/master) for both, _good_ and _bad_ ;) — T.Woody, Oct 31 '14 at 05:33
Print the indicies of the elements actually accessed. From inspection of the code I doubt your producer is behaving in the way you want. — Chris Stratton, Oct 31 '14 at 06:14
@ChrisStratton are you asking me for what is exactly printed in each case? When it is changed to a method that producer calls, the code works (occasional lock, for right now), but it doesn't give the `true` value to `used` this way. — T.Woody, Oct 31 '14 at 06:17
Your code contains some troubling parts. For example, I dont know why this doesn't already give you a segmentation violation: `item_t *item = NULL; item->buf = NULL;` Another thing is that your producer loop sets all elements in the queue to used. Shouldn't that for loop break when it finds the first unused item? — JS1, Oct 31 '14 at 06:19
I'm saying that I don't think your code, even when "working" does what you think it does. I suggest you examine exactly what it is doing with more care, not results but operations, and not to post here but simply to understand. — Chris Stratton, Oct 31 '14 at 06:21
@ChrisStratton I was planning on doing some `isalpha()` kind of stuff at a later point. But, it does work regarding strict plain text documents :) Further optimization can be done, that is verbatim. I guess I was wondering this weird behavior more than anything. — T.Woody, Oct 31 '14 at 06:23
I just looked at the good version. You can see that it returns when it finds the first unused item. In the bad version, you are mising a `break` statement when you find an unused item. — JS1, Oct 31 '14 at 06:25
If you refuse to look into what your program is actually doing, there's no point in discussing it. — Chris Stratton, Oct 31 '14 at 06:26
In the bad version, line #142 writes to a null pointer, because you set item to NULL on line #141. — JS1, Oct 31 '14 at 06:27
@JS1 I think you solved the problem! I was missing that entirely! Thank you for the obvious answer. I'll try to run that now and see if it gives us the same result. — T.Woody, Oct 31 '14 at 06:29
@ChrisStratton I honestly don't mean to come off that way, so I apologize for that. I don't mean to come off so defensive about the code. — T.Woody, Oct 31 '14 at 07:07

score 1 · Accepted Answer · answered Oct 31 '14 at 06:29

1

In the bad version, you need to add a break; after line 150.

answered Oct 31 '14 at 06:29

JS1

4,745
1
14
19

Nailed it! I believe a zen quote of overlooking the obvious is needed right now. – T.Woody Oct 31 '14 at 06:34
1

A second pair of eyes can really help. Most of us develop blind spots to our own code because we know what we intended to write, so we overlook little mistakes where we didn't write what we intended. What you can do for next time is to learn how to use a debugger to step through your code, so that you can spot the place where the code doesn't do what you think it should do. – JS1 Oct 31 '14 at 06:44

Weird issue with returning empty struct instead of NULL

Context

Problem

1 Answers1