Qt objects - am I overusing QMutexLocker?

I have a Qt object that's used by a GUI thread and a networking thread. It looks like:

QString User::Username()
{
    QMutexLocker locker(&mutex);
    return username;
}

void User::SetUsername(const QString &newUsername)
{
    QMutexLocke开发者_C百科r locker(&mutex);
    username = newUsername;
}

QString User::Password()
{
    QMutexLocker locker(&mutex);
    return password;
}
...

Both the GUI and networking thread may use the object (e.g. to display the username on the screen, and to get the username to send across the network).

I'm worried something is wrong, as every method in the object has a QMutexLocker line, to make it thread safe.

Is it acceptable to use QMutexLocker in this way, or is the code structured badly?

You should be using QReadWriteLock and QReadLocker or QWriteLocker respectively. So no threads will be locked if there are only reading threads.

If there are some fields of the class which are accessed changed very frequently, and which dont change any other state of the class, you might want to give it its own dedicated lock.

I think you may be going about things the wrong way. Serializing each method call will "sort of" work, but it won't reliably handle operations like adding or removing a User object. For example, if your main thread deletes the User object, it won't matter that the network thread is carefully locking a mutex, because after the mutex-lock operation returns, the network thread will then try to access the (now deleted) User object, and trying to read OR write freed memory will cause your app to crash (or worse, just mysteriously do the wrong thing sometimes).

Here's a better way to do it (assuming that the User objects are reasonably small): Instead of having the network thread and the I/O thread share the same User object, and trying to serialize all accesses to the object at the method level, you'd be better off giving a separate copy of each User object to the I/O thread. Then when one thread changes its local copy of the User object, it should send a message to the other thread containing a copy of the updated object, and when the other thread receives the message it can update its local copy to match again. That way each thread has exclusive read/write access to its own local set of User objects, and can read/write them without any locking. This also allows each thread to add or remove objects at will (as long as it sends an update-message to the other thread afterwards, so the other thread will follow suit).

I think a better and cleaner way would be to have a "safe section"

updateUser( User ) {

  User.acquireLock()
  User.SetUsername(newUsername)
  User.Password()
  < more operations here >
  User.releaseLock()

}

The advantages of this is that you are locking only once the mutex( that is an expensive operation).