Strange exception.

[VRonin]

@jenya7 said in Strange exception.:

g_queue.enqueue(udp_buffer);

in a separate thread

QByteArray message = g_queue.dequeue();

This is unguarded access to g_queue. It's a race condition. you should use a QMutex or a QSemaphore to police access from different threads

[jenya7]

@VRonin
I see. Where I should place a QMutex ?

[JonB]

@jenya7
@VRonin types much faster than I can! ;-)

Completely unrelated comment:

void PARSER::Run()
{
    while(1)

Are you aware not to use this "for real"? Have you looked at the load on (some thread/core in) your CPU?

[VRonin]

@jenya7 said in Strange exception.:

@VRonin
I see. Where I should place a QMutex ?

Depends, where do you define g_queue?

[jenya7]

@JonB said in Strange exception.:

@jenya7
@VRonin types much faster than I can! ;-)

Completely unrelated comment:

void PARSER::Run()
{
    while(1)

Are you aware not to use this "for real"? Have you looked at the load on (some thread/core in) your CPU?

How it could be done without while(1)? I want to constantly check if there are some data in queue.

[jenya7]

@VRonin said in Strange exception.:

@jenya7 said in Strange exception.:

@VRonin
I see. Where I should place a QMutex ?

Depends, where do you define g_queue?

in udp.cpp
QQueue<QByteArray> g_queue;

in udp.h
extern QQueue<QByteArray> g_queue;

in parser.cpp
#include "udp.h"

[VRonin]

Oh, it's a global.... terrible idea but this is an argument for another time. The mutex will go in the same scope

[jenya7]

@VRonin said in Strange exception.:

Oh, it's a global.... terrible idea but this is an argument for another time. The mutex will go in the same scope
OK.
in udp.cpp

void UDP::ReadyRead()
{
    QHostAddress sender;
    quint16 senderPort;
    int size;

    int count = 0;

   g_mutex.lock();

    while (socket->hasPendingDatagrams())
    {
          socket->readDatagram(udp_buffer.data(), udp_buffer.size(),
                               &sender, &senderPort);

          g_queue.enqueue(udp_buffer);
          count++;
    }

    g_mutex.unlock();

    qDebug() << "datagrams: " << count;
}

and what in void PARSER::Run() ? There is no flag in mutex like g_mutex.isFree() or something.

[JonB]

@jenya7 said in Strange exception.:

How it could be done without while(1)? I want to constantly check if there are some data in queue.

Your code does:

void PARSER::Run()
{
    while(1)
    {
        if (!g_queue.isEmpty())
        {

That means you are calling g_queue.isEmpty() as fast/many times as your thread/core/CPU can manage it. Even though it's in its own thread, which at least (theoretically/hopefully) means that some time will still be given to your UI/other threads, it will/should basically occupy a complete core running 100% of the time. Good if you want to use your host device as a "heater" for your room, bad if you care about electricity/battery life/wear & tear! And it may interfere with the responsiveness of your other threads.

You have a couple of possibilities:

• Make your thread run a Qt event loop (this is the default for QThread), so that you can use QTimer. Only read the queue state every so often in time, to give the thread a chance to sleep. You can achieve this simply using QThread::msleep().

• Again with a Qt event loop in the thread. Have a signal raised by whatever puts something onto the queue, and the thread has a slot for that which only needs to do:

while (!g_queue.isEmpty())
    dequeue_and_process();

and then the slot exits, returning to the thread's event loop.

In this case the second option is very easy, because you know when you put something new onto the queue: it's the g_queue.enqueue(udp_buffer); in your UDP thread. So just have that emit a signal immediately after queueing a new element.

[jenya7]

@JonB
Good points. May in this case I don't need any mutex?

[JonB]

@jenya7 said in Strange exception.:

May in this case I don't need any mutex?

No, you will still need that. Partly for safety anyway, but mostly even after the UDP thread adds to the queue and emits a signal, you still have a potential race condition between (a) the parser thread pulling the item from the queue as soon as the slot is called versus (b) the UDP thread going round and receiving a new datagram to append to the queue.

[jenya7]

@JonB
How do I do that
the first thread

 g_mutex.lock();
//do some stuff
g_mutex.unlock();

and the second one - how to I test the mutex for unlocked?

[J.Hilk]

@jenya7
with https://doc.qt.io/qt-5/qmutex.html#tryLock

it will wait, until timeout or until the other thread called unlock

[jenya7]

@J-Hilk said in Strange exception.:

@jenya7
with https://doc.qt.io/qt-5/qmutex.html#tryLock

it will wait, until timeout or until the other thread called unlock

So if in the second thread tryLock returns true - should I unlock immediately after g_queue.dequeue(); ?

[JonB]

@jenya7
In this particular case your parser thread could actually do an unconditional g_mutex.lock(); too. That would block if the UDP thread has already taken the lock. But because you have "nothing else to do" in your thread it doesn't matter anyway.

However, you may prefer to use @J-Hilk's suggestion of tryLock(), as it may sound more "intuitive". There are other cases --- where your thread does have other things to do if a lock cannot be taken immediately --- where you would require tryLock(), so it may be a habit you want to get into.

[JonB]

@jenya7 said in Strange exception.:

So if in the second thread tryLock returns true - should I unlock immediately after g_queue.dequeue(); ?

Yep :) And don't forget to do so immediately after dequeuing, not after you have processed the element.

[jenya7]

Thank you guys. With mutex it works good. Sometimes I get broken datagrams but it seems like I overload the network card. For 27500 packets I get 11 broken.