The problem found

During the maintenance of project QPerf, a BUG was found. BUG information is as follows:

There is a member variable in the class WorkRequestState, QVector

m_Records, whose value is initially null and the Record instance is added to M_Records when the task starts. The Record class contains a flag attribute that is directly bound to the index of the current instance in m_Records. For example, if the flag of the first Record is 0, then the index in m_Records is 0. At the same time, WorkRequestState object will wait for other threads to trigger void WorkRequestState: : onReply (const ReplyPacket & pack) method. In this method, the received ReplyPacket has attribute flags that correspond one to one to the flags added to m_Records. The method first validates the length of m_Records. The Record object is then removed from it for processing. M_record.size () has a certain probability of being negative.

repeat

If you’re not familiar with QPerf’s business, the last section might be confusing, but that’s okay. In this section, I’ll reproduce this problem using Qt’s built-in unit testing framework, QTest. The ProvideTest class provides the run method as the call method of the test container (QVector and QList). In this class, the QThreadPool multi-threaded calls WriteRunner and ReadRunner are enabled to achieve parallel access to the container, respectively. The test code is shown below.

The test code

providetest.h

#ifndef PROVIDETEST_H
#define PROVIDETEST_H

#include <QList>
#include <QVector>


/** * @brief The ProvideTest class * Qt container thread safety test. * /
class ProvideTest
{
public:
    ProvideTest(a);void run(a);
private:
    QVector<int> m_list;
    QList<int> m_list2;
    friend class ReadRunner;
    friend class WriteRunner;
};

#endif // PROVIDETEST_H 
Copy the code

providetest.cpp

#include "providetest.h"

#include <QTest>
#include <QThreadPool>
/** * @brief The ReadRunner class * Reads The thread's object. * /
class ReadRunner: public QRunnable{
public:
    ReadRunner(int total, ProvideTest *owner);
    // QRunnable interface
public:
    void run(a);
private:
    ProvideTest *m_owner;
    int m_total;
};
/** * @brief The WriteRunner class * The thread object to write to. * /
class WriteRunner: public QRunnable{
public:
    WriteRunner(int total, ProvideTest *owner);
    // QRunnable interface
public:
    void run(a);
private:
    ProvideTest *m_owner;
    int m_total;
};
ProvideTest::ProvideTest() {}void ProvideTest::run(a)
{
    int total = 10000000; // The more tests, the easier it is to reproduce.
 
    ReadRunner *read = new ReadRunner(total, this);
    QThreadPool pool;
    pool.start(read);

    WriteRunner *write = new WriteRunner(total, this);
    pool.start(write);
}

void ReadRunner::run(a)
{
    int count = m_total;
    auto fun = [](auto &item)
    {
        int size = item.size(a);if(size < 0)
        {
            QString msg = QString("Failed size:%1; cur size:%2").arg(size).arg(item.size());
            QFAIL(msg.toUtf8().data()); }};for(int i = 0; i < count; i++){
        fun(m_owner->m_list);
        fun(m_owner->m_list2); }}void WriteRunner::run(a)
{
    int count = m_total;
    for(int i = 0; i < count; i++){
        m_owner->m_list.append(i);
        m_owner->m_list2.append(i);
    }
}

ReadRunner::ReadRunner(int total, ProvideTest *owner)
{
    m_total = total;
    m_owner = owner;
}

WriteRunner::WriteRunner(int total, ProvideTest *owner)
{
    m_total = total;
    m_owner = owner;
}
Copy the code

The test results

Put the test code into the test framework call

void perf::test_qlist(a)
{
    ProvideTest test;
    test.run(a); }Copy the code

The test results are as follows:



From the output of the test results, it can be seen that m_list.size() is negative on the first access and positive on the second. ##:mushroom: problem finding review code, determine that there is no possible stack overflow. The problem may be the QList(QVector) itself. Spent some time reading the QVector source code. Analysis.

The following source code is extractedC: \ Qt \ Qt5.12.9\5.12.9 \ mingw73_64 \ include \ QtCore \ qvector. H, Chinese notes for my convenience to read added. D is a class member of QVector of type QTypedArrayData. Size source is very simple, directly return d size member.inline int size() const { return d->size; }

Append source code is as follows, also a bunch of operations on member D.

template <typename T>
void QVector<T>::append(const T &t)
{
    const bool isTooSmall = uint(d->size + 1) > d->alloc;  // Find if the size is greater than the pre-allocated size after joining
    if (!isDetached() || isTooSmall) {
        T copy(t);
        QArrayData::AllocationOptions opt(isTooSmall ? QArrayData::Grow : QArrayData::Default);
        reallocData(d->size, isTooSmall ? d->size + 1 : d->alloc, opt); // reassign

        if (QTypeInfo<T>::isComplex)
            new (d->end()) T(qMove(copy));
        else
            *d->end() = qMove(copy);

    } else {
        if (QTypeInfo<T>::isComplex)
            new (d->end()) T(t);
        else
            *d->end() = t;
    }
    ++d->size;
}
Copy the code

The basic execution process of the above code is as follows:

  • ReallocaData is called to reallocaData to reallocate memory when the pre-allocated list cannot insert new entries.
  • Initial new data item allocation.
  • Reset the data length.

There seems to be no problem in the main process, it is likely to be reallocData method, follow up the source code of reallocData method.

ReallocData method source code

template <typename T>
void QVector<T>::reallocData(const int asize, const int aalloc, QArrayData::AllocationOptions options)
{
    Q_ASSERT(asize >= 0 && asize <= aalloc);
    Data *x = d;

    const bool isShared = d->ref.isShared(a);if(aalloc ! =0) {
        if(aalloc ! =int(d->alloc) || isShared) {
            QT_TRY {
                // allocate memory
                x = Data::allocate(aalloc, options);
                Q_CHECK_PTR(x);
                // aalloc is bigger then 0 so it is not [un]sharedEmpty
#if! defined(QT_NO_UNSHARABLE_CONTAINERS)
                Q_ASSERT(x->ref.isSharable() || options.testFlag(QArrayData::Unsharable));
#endif
                Q_ASSERT(! x->ref.isStatic());
                x->size = asize;

                T *srcBegin = d->begin(a); T *srcEnd = asize > d->size ? d->end() : d->begin() + asize;
                T *dst = x->begin(a);if(! QTypeInfoQuery<T>::isRelocatable || (isShared && QTypeInfo<T>::isComplex)) { QT_TRY {if(isShared || ! std::is_nothrow_move_constructible<T>::value) {// we can not move the data, we need to copy construct it
                            while(srcBegin ! = srcEnd)new (dst++) T(*srcBegin++);
                        } else {
                            while(srcBegin ! = srcEnd)new (dst++) T(std::move(*srcBegin++)); }}QT_CATCH(...). {// destruct already copied objects
                        destruct(x->begin(), dst); QT_RETHROW; }}else {
                    ::memcpy(static_cast<void *>(dst), static_cast<void *>(srcBegin), (srcEnd - srcBegin) * sizeof(T));
                    dst += srcEnd - srcBegin;

                    // destruct unused / not moved data
                    if (asize < d->size)
                        destruct(d->begin() + asize, d->end());
                }

                if (asize > d->size) {
                    // construct all new objects when growing
                    if(! QTypeInfo<T>::isComplex) { ::memset(static_cast<void *>(dst), 0, (static_cast<T *>(x->end()) - dst) * sizeof(T));
                    } else {
                        QT_TRY {
                            while(dst ! = x->end())
                                new (dst++) T(a); }QT_CATCH(...). {// destruct already copied objects
                            destruct(x->begin(), dst); QT_RETHROW; }}}}QT_CATCH(...). { Data::deallocate(x);
                QT_RETHROW;
            }
            x->capacityReserved = d->capacityReserved;
        } else {
            Q_ASSERT(int(d->alloc) == aalloc); // resize, without changing allocation size
            Q_ASSERT(isDetached());       // can be done only on detached d
            Q_ASSERT(x == d);             // in this case we do not need to allocate anything
            if (asize <= d->size) {
                destruct(x->begin() + asize, x->end()); // from future end to current end
            } else {
                defaultConstruct(x->end(), x->begin() + asize); // from current end to future end} x->size = asize; }}else {
        x = Data::sharedNull(a); }if(d ! = x) {if(! d->ref.deref()) {
            if(! QTypeInfoQuery<T>::isRelocatable || ! aalloc || (isShared && QTypeInfo<T>::isComplex)) {// data was copy constructed, we need to call destructors
                // or if ! alloc we did nothing to the old 'd'.
                freeData(d);
            } else {
                Data::deallocate(d); // release d here. There is no protection.
            }
        }
        d = x;
    }

    Q_ASSERT(d->data());
    Q_ASSERT(uint(d->size) <= d->alloc);
#if! defined(QT_NO_UNSHARABLE_CONTAINERS)
    Q_ASSERT(d ! = Data::unsharableEmpty());
#endif
    Q_ASSERT(aalloc ? d ! = Data::sharedNull() : d == Data::sharedNull());
    Q_ASSERT(d->alloc >= uint(aalloc));
    Q_ASSERT(d->size == asize);
}
Copy the code

The reasons causing

After reallocating memory, d will be freed and then the newly allocated address will be assigned to D, which is not thread-safe. That is, QVector itself is not thread-safe. Its append and size methods do not support concurrent access when memory is reallocated.

Verify the guess

Use QtCreator to set the breakpoint at C:\Qt\Qt5.12.9\5.12.9\mingw73_64\include\QtCore\qvector.h:641 C:\Qt\Qt5.12.9\5.12.9\mingw73_64\include\QtCore\qvector. H :643 and debug the project.

  1. When the breakpoint is broken for the first time, add an expression in QtCreator’s Expressionsd->sizeAnd record its value.



2. F5 continues to the next breakpoint and records the value of Expressions.



3. Step F10 to record the value of Expressions.



By comparing the values of the three executions, a conclusion can be obtained.

When the code executes to Step 2, calling the QVector<T>::size() method returns an incorrect value. When the code executes to Step 3, it returns the normal number.

QList problems are similar to QVector, interested friends can follow the source code, not here.

The solution

The solutions are as follows:

  1. Lock append method and size method. Avoid multithreading insecurity. However, after locking, the performance will decrease linearly. This method is not recommended for high performance requirements.
  2. Preallocate enough memory. It depends on the specific business. If the required memory is determined (as in the case of this request business for the QPerf project), pre-allocating enough memory does not cause memory reallocation and therefore does not enter the pre-allocated method. QVector and QList both provide a pre-allocated constructor (int size). Methods such as reserve() and resize() are also provided to achieve pre-allocated memory.

In QPerf, I used the Reserve method to pre-allocate memory. Before the test starts, I allocate enough memory first, as detailed in the unit tests: ProvideTest.h, provideTest.cpp

conclusion

One lesson learned from this incident is that before using multithreaded shared data access, it is best to find out whether the relevant variables used are thread-safe. (Multithreading is always scary)