Thread-safe implementation of is_empty in a queue datastructure

Question

I am trying to create a thread-safe queue, and while the bulk of the operations

 queue *queue_create_empty(void);
 void queue_enqueue(queue *q, const void *value, const size_t value_size);
 void *queue_dequeue(queue *q);
 void queue_destroy(queue *q, void (*freefunc)(void *value));

I am finding one to be particularily elusive,

 bool queue_is_empty(const queue *q);

In particular if the function definition is

 bool queue_is_empty(const queue *q) {
     assert(q);

     pthread_mutex_lock(q->mutex);
     bool ret = q->is_empty;
     pthread_mutex_unlock(q->mutex);

     /* 
      * Here there is a possibility of a race condition, the queue may actually
      * be empty at this point, and therefore the return value is misleading
      */
     return ret;
 }

then there is a possibility of a race-condition. The last comment

Here there is a possibility of a race condition, the queue may actually be empty at this point, and therefore the return value is misleading

describes the issue.

I have defined my datastructure as such,

 typedef struct queue {
     element *head;
     element *tail;

     /* Lock for all read/write operations */
     pthread_mutex_t *mutex;

     /* Condition variable for whether or not the queue is empty */
     /* Negation in variable names is poor practice but it is the */
     /* only solution here considering the conditional variable API */
     /* (signal/broadcast) */
     pthread_cond_t *not_empty;

     /* Flag used to gauge when to wait on the not_empty condition variable */
     bool is_empty;

     /* A flag to set whenever the queue is about to be destroyed */
     bool cancel;
 } queue;

And for the implementation of the other functions I have worked around the inadequate queue_is_empty function by only checking the value of q->is_empty since I have already taken a lock on the structure before checking the value.

The queue_dequeue function serves as an example of where one would want to know if the queue is empty. See the first if-statement.

 void *queue_dequeue(queue *q) {
     assert(q);

     void *value = NULL;
     pthread_mutex_lock(q->mutex);

     /* We have a mutex-lock here, so we can atomically check this flag */
     if (q->is_empty) {
         /* We do not have to check the cancel flag here, if the thread is awoken */
         /* in a destruction context the waiting thread will be awoken, the later */
         /* if statement checks the flag before modification of the queue */

         /* This allows other threads to access the lock, thus signalling this thread */
         /* When this thread is awoken by this wait the queue will not be empty, or */
         /* the queue is about to be destroyed */
         pthread_cond_wait(q->not_empty, q->mutex);
     }

     /* We have a mutex lock again so we may atomically check both flags */
     if (!q->cancel && !q->is_empty) {
         value = q->head->value;
         if (q->head->next) {            
             q->head = q->head->next;
             free(q->head->previous);

             /* Make dereferencing dangling pointers crash the program */
             q->head->previous = NULL;
         } else {
             free(q->head);
             q->head = NULL;
             q->is_empty = true;
         }        
     }

     pthread_mutex_unlock(q->mutex);
     return value; 
 }

How do I expose a thread-safe queue_is_empty function?

Assuming the other implementations are correct the queue is still thread-safe. "Race condition" is used in the logical sense, from a C standard point of view there is no race condition there. You will have the same issue with a queue_get_size() function. While it returns the size at the time, is may change at any moment. — nwp
– nwp, Commented Jun 29, 2016 at 9:22
Queuing const void * and dequeuing void * looks like an error. — nwp
– nwp, Commented Jun 29, 2016 at 9:23
@nwp I appreciate you pointing that out, I have somehow forgotten that const is a valid qualifier even when specifying the type of a return value. — Filip Allberg
– Filip Allberg, Commented Jun 29, 2016 at 10:14

Art · Accepted Answer · 2016-06-29 09:42:16Z

2

You've just discovered the reason why locking, threading and concurrency in general is hard. It's easy to create a few wrapper functions that obtain and release locks and prevent crashes from data corruption, it's actually hard to get locking right when you depend on state from earlier accesses.

I'd argue that the function queue_is_empty is incorrect because it exists at all. It cannot possibly return a useful value because, as you discovered, the return value is stale before you return it. Since the function cannot return a useful return value it should not exist. And this is where you need to think hard about the API you provide.

One option is to make the locking the responsibility of the callers. The caller could have something like:

queue_lock(q);
if (!queue_is_empty(q))
    do_something(q);
queue_unlock(q);

You will then run into issues with error handling and returns from functions, lock state changes around functions not part of the queue interface, etc. And as soon as you have more than one of those locks you need to manage lock ordering to prevent deadlocks.

Another option is to reduce the API to only provide safe operations. Enqueue and dequeue work correctly. Do you actually need is_empty? What is it good for? Is it just to avoid waiting for a queue element when the queue is empty? Can you not solve it with dequeue_without_waiting instead? Etc.

answered Jun 29, 2016 at 9:42

Art

20.5k1 gold badge40 silver badges66 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Filip Allberg Over a year ago

These are excellent suggestions that I will take to heart. I haven't really decided what, if anything, it is good for, I am essentially porting an earlier queue implementation of mine that was for single-threaded use and therefore I "inherited" the API.

fuz · Accepted Answer · 2016-06-29 09:20:33Z

2

Basically, your function is already correct. It's just that the result is outdated the moment you release the lock. In a multi-threaded program, asking whether a concurrent queue is empty is pretty pointless as another thread could enter data into the queue any moment.

answered Jun 29, 2016 at 9:20

fuz

94.7k27 gold badges216 silver badges391 bronze badges

1 Comment

Jim Mischel Over a year ago

... or remove data from the queue at any moment.

Collectives™ on Stack Overflow

Thread-safe implementation of is_empty in a queue datastructure

2 Answers 2

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related