Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

QCache with QSharedDataPointers crashes in multi-threaded code.



  • 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


  • Lifetime Qt Champion

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

    theCache.insert("SomeKey", data);

    Does this really compile?



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


  • Lifetime Qt Champion

    @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.



  • 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?


  • Lifetime Qt Champion

    @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.



  • @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.


  • Lifetime Qt Champion

    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);
    


  • 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.


Log in to reply