QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution

[prex]

I have an issue with a QMutexLocker in my READ and WRITE methods, which seems to block my whole application.

My Q_PROPERTY read/write methods look like this:

QString QGUI::ipv4() {
  QMutexLocker locker(&mutex_);  
  return m_ipv4;
}

void QGUI::setIpv4(QString value) {
  QMutexLocker locker(&mutex_);
  if (m_ipv4 != value) {
    m_ipv4 = value;
    ipv4Changed();
  }
}

I'm using property bindings in Qml to both read and write to this property (i.e. I have a TextField to set the property and another element reading the property). This is not a problem, if READ happens first and then WRITE. However, whenever I call the WRITE method, the GUI blocks forever (without crashing). I assume that once the ipv4Changed() signal is emitted, all the property bindings are updated by the Qml engine immediately by calling the READ method. Since there's a nested mutex, this now blocks the execution of the READ method and as a result, the return of the WRITE method.

This assumption is based on the fact that the corresponding Q_Properties are never updated and setIpv4 never returns. However, I'm a little bit surprised that ipv4Changed() is a blocking function. I was assuming that the corresponding slots of ipv4Changed (implicitly created by the Q_Property system) just get scheduled in the execution loop.

If you're wondering why I'm using the QMutexLockers: We're running a near real-time application in another QThread. I don't want to run the Qt execution loop there, but I still need to access the Q_Properties of the main/GUI thread. I found using a mutex in the getter/setter functions and directly accessing them from the other thread really neat. Do you see any better solution?

[SGaist]

Recommends does not mean mandate.
If your setter would not emit a signal, then QMutexLocker is the right tool. Since you are emitting a signal, standard mutex manipulation applies or this could be an alternative:

void QGUI::setIpv4(QString value) {
  bool mustEmit(false);
  { // new scope
      QMutexLocker locker(&mutex_);
      if (m_ipv4 != value) {
        m_ipv4 = value;
        mustEmit = true;
      }
    } // locker is destroyed
    if (mustEmit) {
        ipv4Changed();
    }
}

As for the shortest path, as above you don't need to create the locker on the top of the method but on the top of the scope where you will access the variable you want to protect.

[Christian Ehrlicher]

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

just get scheduled in the execution loop.

No, see Qt::ConnectionType

[SGaist]

Hi,

In the setter, since you are emitting a signal, you have to free the mutex before doing it. QMutexLocker is not the right tool here. Also, you are breaking the rule stating that you should always minimize the code path between the locking and unlocking of a mutex. Here you are invoking a method that will snowball and end up in the deadlock you are experiencing.

[Asperamanca]

@prex
Better to have the "real" data in the thread that's working with it, and use a copy in the gui thread. By using implicit sharing, it's pretty cheap to send updates of the data from the data thread to the gui thread via queued connection.
The time-critical processing then doesn't have to wait for anything, and if the GUI lags behind, it won't be in any way noticeable to the user. Queued events are surprisingly quick to arrive if there's CPU time available, and you'll have to wait for the rendering thread to finish it's work anyway, so there is no "synchronous" GUI with QML anyway.

[prex]

@Christian-Ehrlicher said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

just get scheduled in the execution loop.

No, see Qt::ConnectionType

You're right. Since the receiver lives in the thread that emits the signal, DirectConnection is used:

The slot is invoked immediately when the signal is emitted. The slot is executed in the signalling thread.

Does this mean ipv4Changed() only returns after every connected slot is executed?

@SGaist said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

Also, you are breaking the rule stating that you should always minimize the code path between the locking and unlocking of a mutex. Here you are invoking a method that will snowball and end up in the deadlock you are experiencing.

But why is the QMutexLocker used at all since it never minimizes the code path compared to a QMutex. The Qt Docs clearly recommend QMutexLocker. Do you propose the following?

QString QGUI::ipv4() {
  QMutexLocker locker(&mutex_);  
  return m_ipv4;
}

void QGUI::setIpv4(QString value) {
  QMutexLocker locker(&mutex_);
  if (m_ipv4 != value) {
    m_ipv4 = value;
    **locker.unlock();**
    ipv4Changed();
  }
}

@Asperamanca said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@prex
Better to have the "real" data in the thread that's working with it, and use a copy in the gui thread. By using implicit sharing, it's pretty cheap to send updates of the data from the data thread to the gui thread via queued connection.
The time-critical processing then doesn't have to wait for anything, and if the GUI lags behind, it won't be in any way noticeable to the user. Queued events are surprisingly quick to arrive if there's CPU time available, and you'll have to wait for the rendering thread to finish it's work anyway, so there is no "synchronous" GUI with QML anyway.

But what you propose requires an execution loop in my thread. I'm not concerned about the minimal lag in the GUI, I just don't want to run the execution loop in the thread, since I'm running a while loop with a fixed timing.

[SGaist]

Recommends does not mean mandate.
If your setter would not emit a signal, then QMutexLocker is the right tool. Since you are emitting a signal, standard mutex manipulation applies or this could be an alternative:

void QGUI::setIpv4(QString value) {
  bool mustEmit(false);
  { // new scope
      QMutexLocker locker(&mutex_);
      if (m_ipv4 != value) {
        m_ipv4 = value;
        mustEmit = true;
      }
    } // locker is destroyed
    if (mustEmit) {
        ipv4Changed();
    }
}

As for the shortest path, as above you don't need to create the locker on the top of the method but on the top of the scope where you will access the variable you want to protect.

[jeremy_k]

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

If you're wondering why I'm using the QMutexLockers: We're running a near real-time application in another QThread. I don't want to run the Qt execution loop there, but I still need to access the Q_Properties of the main/GUI thread. I found using a mutex in the getter/setter functions and directly accessing them from the other thread really neat. Do you see any better solution?

Is the datum underlying the property atomic, or otherwise guaranteed by the target platform guaranteed to be consistently accessed across threads? Using a mutex in one thread and not in the other doesn't appear to buy anything useful.

[jeremy_k]

@SGaist said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

void QGUI::setIpv4(QString value) {
  bool mustEmit(false);
  { // new scope
      QMutexLocker locker(&mutex_);
      if (m_ipv4 != value) {
        m_ipv4 = value;
        mustEmit = true;
      }
    } // locker is destroyed
    if (mustEmit) {
        ipv4Changed();
    } 
}

This could be simplified a bit more, and demonstrate the ability of QMutexLocker or any RAII construct to handle multiple paths without additional user code:

> void QGUI::setIpv4(QString value) {
   { // new scope
       QMutexLocker locker(&mutex_);
       if (m_ipv4 == value)
         return; // locker is destroyed, mutex_ released
       m_ipv4 = value;
   } // locker is destroyed, mutex_ released
   emit ipv4Changed();
 }

[Asperamanca]

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

But what you propose requires an execution loop in my thread. I'm not concerned about the minimal lag in the GUI, I just don't want to run the execution loop in the thread, since I'm running a while loop with a fixed timing.

Only in the receiving thread, to my knowledge. That would be the GUI thread, and I assume you are running an event loop there (I doubt UI would work otherwise).

[prex]

@SGaist said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

Recommends does not mean mandate.
If your setter would not emit a signal, then QMutexLocker is the right tool. Since you are emitting a signal, standard mutex manipulation applies or this could be an alternative:

void QGUI::setIpv4(QString value) {
  bool mustEmit(false);
  { // new scope
      QMutexLocker locker(&mutex_);
      if (m_ipv4 != value) {
        m_ipv4 = value;
        mustEmit = true;
      }
    } // locker is destroyed
    if (mustEmit) {
        ipv4Changed();
    }
}

As for the shortest path, as above you don't need to create the locker on the top of the method but on the top of the scope where you will access the variable you want to protect.

Thanks, this makes sense.

@jeremy_k said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

If you're wondering why I'm using the QMutexLockers: We're running a near real-time application in another QThread. I don't want to run the Qt execution loop there, but I still need to access the Q_Properties of the main/GUI thread. I found using a mutex in the getter/setter functions and directly accessing them from the other thread really neat. Do you see any better solution?

Is the datum underlying the property atomic, or otherwise guaranteed by the target platform guaranteed to be consistently accessed across threads? Using a mutex in one thread and not in the other doesn't appear to buy anything useful.

Not sure if I understand. But both threads use the same mutex since they both call the getter/setter method. One over properties, one directly by calling the method.

@jeremy_k said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@SGaist said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

void QGUI::setIpv4(QString value) {
  bool mustEmit(false);
  { // new scope
      QMutexLocker locker(&mutex_);
      if (m_ipv4 != value) {
        m_ipv4 = value;
        mustEmit = true;
      }
    } // locker is destroyed
    if (mustEmit) {
        ipv4Changed();
    } 
}

This could be simplified a bit more, and demonstrate the ability of QMutexLocker or any RAII construct to handle multiple paths without additional user code:

> void QGUI::setIpv4(QString value) {
   { // new scope
       QMutexLocker locker(&mutex_);
       if (m_ipv4 == value)
         return; // locker is destroyed, mutex_ released
       m_ipv4 = value;
   } // locker is destroyed, mutex_ released
   emit ipv4Changed();
 }

But this would emit signals even if the value did not change and if I understand correctly, this is not good practice.

@Asperamanca said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

But what you propose requires an execution loop in my thread. I'm not concerned about the minimal lag in the GUI, I just don't want to run the execution loop in the thread, since I'm running a while loop with a fixed timing.

Only in the receiving thread, to my knowledge. That would be the GUI thread, and I assume you are running an event loop there (I doubt UI would work otherwise).

Indeed, the receiving thread/main thread/GUI thread runs an execution loop. But I think to use slots in the workerThread (the one that shouldn't have an execution loop), I would need an execution loop (since no direct connection across threads).

[JonB]

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

But this would emit signals even if the value did not change and if I understand correctly, this is not good practice.

How do you figure it would emit if unchanged value given that code has:

       if (m_ipv4 == value)
         return;

?

[prex]

@JonB said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

@prex said in QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution:

But this would emit signals even if the value did not change and if I understand correctly, this is not good practice.

How do you figure it would emit if unchanged value given that code has:

       if (m_ipv4 == value)
         return;

?

Whoops, missed that line.

[SGaist]

@jeremy_k even better ! I knew I was missing something in my implementation.

[jeremy_k]

@SGaist Never go first in code golf!

[jeremy_k]

@prex said in [QMutexLocker in Q_PROPERTY READ/WRITE methods blocks execution](/post

Is the datum underlying the property atomic, or otherwise guaranteed by the target platform guaranteed to be consistently accessed across threads? Using a mutex in one thread and not in the other doesn't appear to buy anything useful.

Not sure if I understand. But both threads use the same mutex since they both call the getter/setter method. One over properties, one directly by calling the method.

I wasn't sure if "directly accessing them" meant the getter and setter functions, or the datum. Since it's now clear that it refers to the functions, that implies access to the QObject instance. In that case, be aware that very few of QObject's functions are thread-safe.