Concurrent access to data with QReadWriteLock

Question

Using Qt, I've got this code in order to protect access to some shared data between threads. I'm pretty sure the idea is correct, but I don't know if RVO and/or RAII could potentially screw the get function. I'm much more used to C and while I understand the idea, I'm not totally familiar with all the "gotchas" for these two concepts.

class DataManager {
 Q_OBJECT
private:
 QVector<DataType> data;
 QReadWriteLock* rwLock;
public:
 DataManager() {
 rwLock = new QReadWriteLock();
 }
 ~DataManager() {
 delete rwLock;
 }
 Q_DISABLE_COPY(DataManager)
 QVector<DataType> getData() {
 QReadWriteLocker lock(rwLock);
 return data;
 }
 QVector<DataType>* beginModifyData() {
 rwLock->lockForWrite();
 return &data;
 }
 void endModifyData() {
 rwLock->unlock();
 emit dataChanged();
 }
signals:
 void dataChanged();
};

In the get function, is it possible that the RAII-type class QReadWriteLocker unlocks the lock before the return copy is made? Thus allowing some thread, that was waiting to write, to overwrite the data being returned.

Also, if somebody writes

QVector<DataType>& myData = dataManager->getData();

Is it possible, due to RVO, that they get a reference to the actual data?

Also, I'd like to receive comments on the code and idea itself. Below I've outlined the reasons why I chose this approach.

1. Simple to use. Since one can only read a copy of the data, they don't have to worry about synchronization and locking.
2. Taking advantage of the implicit sharing of Qt containers, no actual copies will ever be made when using getData(), thus making read-only access very fast.
3. Since, when modifying the data, one gets a pointer to the actual data, again, in most cases, no copies will be made, unless some thread is caching the data (or one of the copies has not yet gone out of scope of whatever got it).
4. Looking at the documentation and code, a non-recursive QReadWriteLock is ultra fast in the non-contended case, not doing any waiting or context-switching (uses atomic operations on an integer).
5. I could potentially do some data validation/fixup in the endModifyData function if needed.

Thanks.

Answer 1

Locking

Let's see what these member functions actually do, and what can go wrong:

getData

The lock acquired in getData basically only ensures that nobody is currently modifying data while inside. As soon as the caller gets the returned value, another thread is free to modify it.

To be a bit clearer, this is how RAII breaks down:

QVector<DataType> getData() {
 QReadWriteLocker lock(rwLock);
 QVector<DataType> temp = data;
 lock.~QReadWriteLocker(); // unlock
 return temp;
}

So the lock doesn't actually cover subsequent usage.

beginModifyData/endModifyData

These two member functions expect to be called in tandem. But what happens if the caller to beginModifyData forgets to call endModifyData (e.g. because an exception was thrown)? Obviously, in that case the lock never gets released.

Also, somebody might store the returned pointer, thus having unsynchronized access to the data after the lock got released.

General stuff

Is there any specific reason why rwLock has to be a pointer? It could simply be of type QReadWriteLock instead.

Is there any specific reason why beginModifyData returns a QVector<DataType>* instead of a QVector<DataType>&?

Your question regarding RVO

No, that is not how return value optimization (RVO) works. For a better understanding, if the compiler applies RVO it might conceptually rewrite the code to something similar to this:

void getData(QVector<DataType>* return_value) {
 QReadWriteLocker lock(rwLock);
 // construct a new QVector<DataType> at the given address (to be passed from the callsite)
 new(return_value) QVector<DataType>(data);
}
// the callsite would be rewritten from
QVector<DataType> myData = dataManager->getData();
// to
QVector<DataType> myData; // uninitialized!
dataManger->getData(&myData);

So this obviously won't work with a reference.

Without RVO, the reference won't work at all, as the lifetime of the returned object ends at the end of its declaration. However, it will work (at least partially) for a const QVector<DataType>& as lifetimes of temporaries get extended for those (though it will still refer to the copy).

• hoffmale: You might want to take a look at this talk to get an idea for a more general approach.
• Raziel: Thanks for your input. About getData, I don't see how that would be a problem. The caller gets a copy of the returned data (an implicitly shared copy), so "subsequent usage" has no concurrency whatsoever. As long as QReadWriteLocker's destructor is called after the copy is made, which your answer confirms, there should be no problem. Now, if DataType could not be copied (eg. has some resource handles/objects), then the solution in that excellent talk you shared would be the way to go.
• Raziel: About the *modifyData functions, I know that this is a possible issue. However, I'm not trying to protect against maliciously exploiting the code, I'm trying to protect against accidentally using it wrong. If someone forgets to do endModifyData, the program would almost immediately deadlock; then a "debug/break" would show the waiting thread; then a simple git diff | grep beginModifyData would point to the problem. Somebody keeping the pointer could be bad, but any half-decent programmer knows that keeping pointers to stuff you don't own is evil, I'm not too concerned about that.
• Raziel: About the general stuff: rwLock doesn't have to be a pointer. I was thinking of changing it myself before asking the question. beginModifyData returns a pointer mostly because of convention and coding style. Read-only data/objects are passed around with const references; mutable stuff, expected to be changed, is passed around with pointers. We try to avoid non-const references. Easier to read, tends to be less bug-prone, etc. Subjectively.
• Raziel: About the RVO thing: you know, I actually knew that. I guess knowing it and applying it properly are two different things. Thanks for your clarification.