2

In the book "C++ Concurrency in Action" by Anthony Williams, when describing the listing "7.9 A lock-free stack using a lock-free std::shared_ptr<>", the author states that we have to:

to clear the next pointer from the popped node in order to avoid the potential for deeply nested destruction of nodes when the last std::shared_ptr referencing a given node is destroyed

template <typename T>
class lock_free_stack
{
private:
  struct node
  {
      std::shared_ptr<T> data;
      std::shared_ptr<node> next;
      node(T const &data_) : data(std::make_shared<T>(data_)){}
  };
  
std::shared_ptr<node> head;

public:

  void push(T const &data)
  {
      std::shared_ptr<node> const new_node = std::make_shared<node>(data);
      new_node->next = std::atomic_load(&head);
      while (!std::atomic_compare_exchange_weak(&head,&new_node->next, new_node));
  }
  std::shared_ptr<T> pop()
  {
      std::shared_ptr<node> old_head = std::atomic_load(&head);
      while (old_head && !std::atomic_compare_exchange_weak(&head, &old_head,         std::atomic_load(&old_head->next)));
      if (old_head)
      {
          std::atomic_store(&old_head->next, std::shared_ptr<node>());
          return old_head->data;
      }
      return std::shared_ptr<T>();
  }
  ~lock_free_stack()
  {
      while (pop())
          ;
  }
};

The line he's referring to is this one:

std::atomic_store(&old_head->next, std::shared_ptr<node>());

I don't see why that line is needed since the node pointed by old_head->next is also pointed by std::shared_ptr<node> head; or another thread executing pop(), therefore there will be still one shared pointer pointing to the node so the node won't be deleted.

Am I missing something, or is my view correct?

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
Maurice
  • 31
  • 3
  • Once the `old_head` is `pop()`'d, why should it's `next` still be pointing at something? For example, imagine having a million nodes. You pop a node, and keep it around forever. Then pop the remaining million-minus-one nodes. That first held onto node will also be holding onto the rest of the mesh of nodes, even though it's been popped, and otherwise have no relation to one another anymore. – Eljay Jun 05 '23 at 18:37
  • Does this code even compile? The `std::atomic_...()` functions expect `std::atomic` objects, but there are no `std::atomic` objects anywhere in this code. If you look at cppreference's doc for [`std::atomic_compare_exchange_weak()`](https://en.cppreference.com/w/cpp/atomic/atomic_compare_exchange), for example, it uses `std::atomic` instead of `std::shared_ptr`. AFAIK, `shared_ptr` already has its own thread safety built-in, you shouldn't need to use `std::atomic` with it. – Remy Lebeau Jun 05 '23 at 20:13
  • 1
    @RemyLebeau: The control block of a `shared_ptr` is accessed using atomic ops, but the actual `shared_ptr` object itself isn't automatically thread-safe. See https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic re: its overloads of `std::atomic` functions like load. – Peter Cordes Jun 05 '23 at 20:23

1 Answers1

0

This implementation is not valid, you need to use std::atomic<std::shared_ptr<node>> for the head. Clearing old_head->next is not nedded because old_head is destroyed after function goes out of scope.

Pebal
  • 41
  • 1
  • 1