0

I know it isn't thread-safe to use a normal list as spsc-queue. But I don't know why? What's wrong will happen if I do this? I read a paper about mpsc-queue, and I do some changes to the code. I change it to spsc-queue. But I don't know if my spsc_queue is thread_safe.

template <typename T>
class SPSCQueue
{
    struct Node
    {
        T *item = nullptr;
        Node *next = nullptr;
        bool is_sentinel;

        Node() : is_sentinel(true) {}
    };
public:
    SPSCQueue() 
    {
        m_head = new Node;
        m_tail = m_head;
    }

    ~SPSCQueue()
    {
        Node *tmp = m_head;
        while (m_head != nullptr)
        {
            temp = m_head;
            m_head = m_head->next;
            delete temp;
        }
    }

    void Enqueue(T *item)
    {
        if (item == nullptr) return;
        Node *last_tail = m_tail;
        m_tail = new Node();
        last_tail->item = item;
        last_tail->next = m_tail;
        last_tail->is_sentinel = false;
    }

    T *Dequeue()
    {
        if(m_head->is_sentinel) return nullptr;

        Node *last_head = m_head;
        T *item = last_head->item;
        m_head = last_head->next;
        delete last_head;
        return item;
    }

private:
    Node *m_head = nullptr;
    Node *m_tail = nullptr;
};
Rhysol
  • 481
  • 1
  • 3
  • 12
  • Then you get C++11 data race UB, unless you use `std::atomic`. Compilers will assume that variable values can be kept in registers after inlining, instead of being re-read from memory in case they changed. Plus no ordering. – Peter Cordes Nov 05 '19 at 12:25
  • 1
    @KamilCuk: That's ruled out as a pre-condition: SPSC = "Single Producer, Single Consumer". The danger is consumer racing with producer. – Peter Cordes Nov 05 '19 at 12:26
  • @PeterCordes But my spsc-queue doesn't use inline and I use a sentinel node to prevent data race. Only enqueue is done, dequeue would success. Is there any way to test if my queue is ok? – Rhysol Nov 05 '19 at 12:32
  • I think the `last_tail->is_sentinel` may be reordered before ex. `last_tail->next = m_tail;`. The "doesn't use inline" is not relevant, compiler is allowed to do that. So the test `if(m_head->is_sentinel)` may fail, while `last_tail->next` is not written to. You should use `std::atomic` What does `: ts(true)` do in `Node::Node`? – KamilCuk Nov 05 '19 at 12:44
  • @Rhysol: The `inline` keyword is irrelevant to whether a compiler can actually inline or not. Link-time optimization can do cross-file optimization. Also, you define methods inside the `class {}` definition itself so they're implicitly declared `inline`. To test how broken your queue is: compile producer and consumer functions that make call it in a tight loop, using a counter or something to detect lost messages in case it's not just an infloop, *with optimization enabled*, and see [MCU programming - C++ O2 optimization breaks while loop](//electronics.stackexchange.com/a/387478) – Peter Cordes Nov 05 '19 at 12:58
  • @KamilCuk sorry it's a mistake, I've fixed it. You mean `last_tail->is_sentinel` may execute before `last_tail->next = m_tail` or `m_tail = new Node()` because of compiler? If I use std::atomic instead of bool, the compiler would not do that? – Rhysol Nov 05 '19 at 12:58
  • @PeterCordes Thanks, I'll write some test code. – Rhysol Nov 05 '19 at 13:20
  • If you're curious about exactly how it breaks and want to look at the asm to see how it got stuck, then sure, but your code is 100% broken. You could just read my linked answer to understand the problem. There are some similar duplicates on SO about actual threading, not interrupt-handlers. e.g. a `while(!(tmp=Dequeue())` loop could compile as `if(m_head->is_sentinel) infinite_loop{}` – Peter Cordes Nov 05 '19 at 13:43

0 Answers0