QCache with QSharedDataPointers crashes in multi-threaded code.

Eric Singer

I'm experiencing a crash when using QCache to hold QSharedDataPointers in multi-threaded code. I have a typical QSharedData/QSharedDataPointer structure, as described in the Qt docs:

public class MySharedData : public QSharedData
{
public:
    // data members
    int x;
}

public class MySharedDataHolder
{
public:
    // plain constructor
    MySharedDataHolder()
    {
        d = new MySharedData;
    }
    // copy constructor
    MySharedDataHolder(const MySharedData& other) : d(other.d)
    {
    }
    // assignment operator
    MySharedDataHolder& operator=(const MySharedDataHolder &other)
    {
        d = other.d;
        return *this;
    }
    // destructor
    ~MySharedDataHolder()
    {
        // CRASH! (sometimes!)
        // the crash occurs here, at some point in the process of clearing the cache and destroying instances of MySharedDataHolder
    }
    // accessors
    void SetX(int val)
    {
        d->x = val;
    }
    int GetX()
    {
        return d->x;
    }
    
private:
    // shared data pointer 
    QSharedDataPointer<MySharedData> d;
}

I keep MySharedDataHolder objects in a QCache for use by other threads.

QCache<QString, MySharedDataHolder> theCache;
QReadWriteLock theCacheLock;

Data is accessed and modified by threaded runnable classes

class MyConsumer : public QRunnable
{
public:
    void run() override
    {
        // ...
        MySharedDataHolder data = GetData("SomeKey");
        data.SetX(123);
        // ...
    }

    MySharedDataHolder GetData(const QString& key)
    {
        // request data from cache
        QReadLocker readLock(theCacheLock);
        if (theCache.contains("SomeKey")
        {
            // it's there, so get it
            return = *(theCache["SomeKey"]);
        }

        // it wasn't there, so add it
        readLock.unlock();
        QWriteLocker writeLock;
        MySharedDataHolder* data = new MySharedDataHolder(someOtherData);
        theCache.insert("SomeKey", data);
        return *data;
    }
}

In the course of the program, the cache will be filled and accessed by MyConsumer threads. Presumably, the QSharedDataHolder member "d" should be shared and copied on write by virtue of MyConsumer operations.

When I try to clear the cache with theCache.clear(), it often crashes in the middle of this process, as MySharedDataHolder objects in the cache are destroyed. It doesn't happen all the time, nor on the destruction of the first item in the cache. I've checked this by manually clearing the cache with take() and delete. The error I get is an attempt to access an invalid (previously deleted?) area of the heap.

It tends to happen when the cache has been filled, partially emptied and refilled (normal QCache operation). On rare occassions, it will crash during an insert, but by far, the majority of crashes happen during cache clearing.

If I make MyConsumer a non-threaded class, the crash doesn't occur.

Am I misusing any aspects of these classes, or is there potentially a bug with QCache handling QSharedDataPointers in a threaded context? Any insights would be appreciated.

Eric

Christian Ehrlicher

@Eric-Singer said in QCache with QSharedDataPointers crashes in multi-threaded code.:

theCache.insert("SomeKey", data);

Does this really compile?

Eric Singer

I made a mistake distilling the code to an example. Corrected it to &data. Was that what you were referring to?

Christian Ehrlicher

@Eric-Singer said in QCache with QSharedDataPointers crashes in multi-threaded code.:

Was that what you were referring to?

Yes, and this is wrong: After this call, object is owned by the QCache and may be deleted at any time.

Eric Singer

Understood, but that is the reason for the code which uses 'contains' to check for the object in the cache, and inserts a new copy if it doesn't find it. Is there something wrong there?

Christian Ehrlicher

@Eric-Singer said in QCache with QSharedDataPointers crashes in multi-threaded code.:

and inserts a new copy if it doesn't find it.

You don't create a copy.

Eric Singer

@Christian-Ehrlicher I corrected several more transcription mistakes. I get the object from the cache if it's there and return it by value. If not, I create a new object, insert it in the cache and return it by value.

Christian Ehrlicher

And does it still crash? Because you forgot to lock for read. Returning return *data; is still wrong: In particular, if cost is greater than maxCost(), the object will be deleted immediately.

QReadWriteLocker l(theCacheLock);
l.lockForRead();
auto ret = theCache["someKey"];
if (ret)
  return *ret;
l.lockForWrite();
theCache.insert("someKey", new MySharedDataHolder(42));
return MySharedDataHolder(42);

? Offline

Turns out to be a known bug: https://bugreports.qt.io/browse/QTBUG-19794

While both operator[] and object() are defined as const functions, they are not thread safe. Internally the linked list the cache is using gets rearranged so that the requested object is now on top of the list. Replacing the ReadWriteLock with a Mutex solved the problem.