Bug in QReadWriteLock Non Recursive

SidTheLazy

Hi Folks,

under linux qt 4.7.4 using gcc 4.4.3
the following code compiles fine and gives no runtime error.

@class TestThread: public QThread {
private:
QReadWriteLock mutex;

public:
bool mStop;

TestThread(): mutex(QReadWriteLock::NonRecursive),mStop(false) {}
void run() {
  while(!mStop) {
    mutex.lockForRead();        
    qDebug() << "Tread running";
    msleep(100);        
  }
}

};@

The lock is locked by the same thread several times and nothing happens. According to the manual
http://doc.qt.nokia.com/4.7.1/qreadwritelock.html

the lock should only be lockable once by the same thread. Is this some serious bug or am I misunderstanding the manual ?

andre

I think you misunderstand the class, yes.

You are allowed to lock multiple times for reading. It will only fail once there has been a lock for writing.

Edit:
Hmmm... perhaps I am wrong. Reading the docs seems to suggest that indeed the lock should fail if the same thread tries to lock more than once.

veeeee_d

It doesn't say anything about being able to lock only once per thread, why is it you are assuming that?

SidTheLazy

@Veeee.. Quote from the doc:
QReadWriteLock::Recursive 1 In this mode, a thread can lock the same QReadWriteLock multiple times and the mutex won't be unlocked until a corresponding number of unlock() calls have been made.
QReadWriteLock::NonRecursive 0 In this mode, a thread may only lock a QReadWriteLock once.

@Andre (The Edit):
Exactly that's what I read from this passage as well. It seems odd since this functionality would include book-keeping of the reading threads but the text is quite clear and it is obvious that only one write at a time is possible.

andre

I think you should file a bugreport.

goetz

In my opinion, the bug is in the docs, not the code.

The pure purpose of a read write mutex is to enable multiple reads. The read locks are not recursive but just add to the number of locks (just as if they were read locks from different threads).

The only difference is that a recursive lock must be unlocked the same count than it was locked, while a non-recursive lock must be unlocked only once per thread.

andre

I am not sure where the bug is. Having a non-matching number of locks and unlocks sounds wrong to me as well. I don't see the purpose of the flag for this class at all. I agree with you that the purpose is to enable multiple reads, and the class should not care about if those are from the same thread or not. However, I find it weird not to have to have a matching number of locks and unlocks. That would result in the lock being unlocked at the first unlock call from the thread. If there are multiple reads from the same thread, these would be unsave after that. That doesn't sound right.

veeeee_d

Andre, I believe the issue is that the lock should have the ability to be locked on every read - and those might be on different parts of the same thread, you never know. The bug lies in the flag not allowing you to lock only once in your thread, case in which if you lock in more parts of your code, then the single unlock() call you make will not really be helpful since the other read requests are still pending.

bu7ch3r

"QReadWriteLock mutex" shouldn't be global ? How do you sinc multiple instances of TestThread if each class has his own QRWLock?

Another thing wich may be wrong or stupid:
I think that it could be an compiler optimization problem or at least in visual studio is. Again, I might be wrong: if the compiler detects a while loop on a variable that might not change its value he harcodes that value, I think "mStop" should be volatile.

dangelog

EDITED OUT

After a bit of discussion on #qt-chat, OP's issue is confirmed -- locking for read twice should deadlock a non recursive QReadWriteLock. But actually it doesn't.

But the point is: is anyone relying on this behaviour?

SidTheLazy

@Peppe:
It took me some hours to find out why my code was not behaving like given in the specs when doing unit testing and trying to trigger the deadlock. A non-recursive lock could be useful if one thread can only acquire the lock once to avoid programming bugs (lock the same lock several times from your code and forget to free it the same number of times).
Apart from this, if the flag is utterly useless it should definitely be removed.
One thing is for sure freeing a lock should never result in freeing several locks, but as far as I know this has never been the case.