QQueue, is it thread safe

SPlatten

I want to use a QQueue to queue up messages for delivery over sockets. The question is, are the functions enqueue and dequeue thread safe or do I need to add mutex's around these calls?

I have visited:

https://doc.qt.io/qt-5/qqueue.html#details

And I cannot see any mention of threads.

PaulNewman

It's just a wrapper around QList so it's not safe, if you really wanna queue std::queue will be better solution

J.Hilk

@SPlatten

only reentrant, like stated in the documentation

SPlatten

@PaulNewman , what advantages does std::queue offer over QQueue ?

PaulNewman

@SPlatten look here https://doc.qt.io/qt-5/containers.html#algorithmic-complexity and here https://en.cppreference.com/w/cpp/container/deque

QList<T>

Random access - O(1)
Insertion or removal of elements at the end or beginning - Amort. O(1)
Insertion or removal of elements - O(n)

std::deque:

Random access - O(1)
Insertion or removal of elements at the end or beginning - O(1)
Insertion or removal of elements - O(n)

Note:
"Internally, QList<T> is represented as an array of T if sizeof(T) <= sizeof(void*) and T has been declared to be either a Q_MOVABLE_TYPE or a Q_PRIMITIVE_TYPE using Q_DECLARE_TYPEINFO. Otherwise, QList<T> is represented as an array of T* and the items are allocated on the heap."
In most cases sizeof(T) > sizeof(void*) so queue(deque) will be faster.
In Qt6 QList will be reworked and maybe perfomance will change

aha_1980

@PaulNewman,

you should state that std::queue is not thread safe either, because that was the question of @SPlatten.

Regards

Asperamanca

If you share a reference to the same QQueue between threads, you need to implement proper locking.

However, if you hand a copy of a QQueue to a thread, you are all done.

See https://doc.qt.io/qt-5/threads-modules.html#threads-and-implicitly-shared-classes

mnesarco

It is not thread safe, you cannot enqueue and dequeue concurrently from different thread. I have used it and learned it by the hard way.

kshegunov

@PaulNewman said in QQueue, is it thread safe:

In most cases sizeof(T) > sizeof(void*) so queue(deque) will be faster.

Careful there. Algorithmic complexity is not the same as "faster", a linked list has the same complexity for appending as a regular vector, but the latter is almost certainly faster. The std::deque is a doubly linked list, and as such has reference stability, but also suffers from heap allocating each element and the pointer indirections that come with traversing it.

@SPlatten said in QQueue, is it thread safe:

And I cannot see any mention of threads.

Reentrant from @J-Hilk's screenshot is the cue.

I want to use a QQueue to queue up messages for delivery over sockets. The question is, are the functions enqueue and dequeue thread safe or do I need to add mutex's around these calls?

Use signals and slots and leverage the internal message queue in the QThread class' exec() implementation.

SPlatten

@kshegunov I am using signals and slots, but just because the member variable using QQueue variable is in a slot doesn't make it thread safe or does it?

I originally asked because I am using signals and slots, but do I need to add a mutex in the slot and thread body to protect the QQueue member?

kshegunov

@SPlatten said in QQueue, is it thread safe:

@kshegunov I am using signals and slots, but just because the member variable using QQueue variable is in a slot doesn't make it thread safe or does it?
I originally asked because I am using signals and slots, but do I need to add a mutex in the slot and thread body to protect the QQueue member?

I can't say. Depends on few things:

Are you using the default implementation of QThread::run and thus have you an event loop running there?
Are you connecting the signals/slots with Qt::AutoConnection or are you forcing the connection type? Provide a small snippet out of the slot, please, so we know what we are talking about.

*) A member/variable can't be thread-safe, this is a jargon we use. It means that (all) its class' methods are either reentrant/thread-safe/nonreentrant/non-thread safe. There can be mixes of these for different functions/methods, though.
Data itself is oblivious to threads, the concurrent access is significant for the execution paths (i.e. function calls), so a function/method can be assigned that meaning, not the object itself.

SPlatten

@kshegunov , my connections are typically:

QObject::connect(this, &clsMsgSender::sendJSON
                ,this, &clsMsgSender::onSendJSON);

Here is an example of that slot:

void clsMsgSender::onSendJSON(const QJsonObject &crobjJSON) {
    clsMsgSender* pService = clsMsgSender::pGetService();

    if ( pService == nullptr ) {
    //Shouldn't get here!
        return;
    }
    if ( pService->isRunning() != true ) {
    //Start service
        emit pService->startService();
    }
    QMutexLocker lock(&pService->mMutex);
    pService->mqueMsgsOut.enqueue(crobjJSON);
}

'pService' points to the static instance of the same class which has the QQueue member.

kshegunov

Since you're using a global instance that's touched in multiple places (i.e. threads) you need to protect each and every access. What you should follow up with is the thread safety classics: the consumer-producer problem (there's also an example implementation in Qt's docs).

pService->isRunning() is a race condition unless you took special measures (e.g. using an atomic to set/get the status).

QMutexLocker lock(&pService->mMutex);
Should be inside the pService instance and should be locked unlocked by the class API, ordinarily. You're spilling the class implementation here, which with strong coupling (between classes) is going to bite you sooner or later.

Additionally, please show the clsMsgSender::pGetService too. I suspect you have a problem there as well.

SPlatten

@kshegunov

private:
    static clsMsgSender* mspService;

public:
    static clsMsgSender* pGetService() { return clsMsgSender::mspService; }

kshegunov

Yes, this is fine as long as you guarantee that the construction and destruction are not concurrent between different threads.