Share QString between threads

[Max Logvinov]

Hello.

Can anybody explain me how to share QString between threads? Under "share" I mean that one thread is writing to QString and another thread is reading from it. I know, that QString uses implicit sharing, so it's reentrant only, but not thread-safe. I guess that I have to use some locking when reading from or writing to a QString. Here is example of my code:

// Task.h
class Task : public QObject
{
    Q_OBJECT

public:
    QString errorText() const
    {
        QReadLocker locker(&m_lock);
        return m_errorText;
    }

    void setErrorText(const QString &errorText)
    {
        QWriteLocker locker(&m_lock);
        if (errorText != m_errorText)  {
            m_errorText = errorText;
            locker.unlock();
            emit errorTextChanged();
        }
    }

signals:
    void errorTextChanged();

private:
    QString m_errorText;
    QReadWriteLock m_lock;
};

// MainWindow.h
class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    void executeTask(int taskIndex)
    {
        auto worker = new Worker(m_tasks[taskIndex]);
        QObject::connect(worker, &Worker::finished, worker, &QObject::deleteLater);
        worker->start();
    }

private:
    QList<QSharedPtr<Task>> m_tasks;
};

// Worker.h
class Worker : public QThread
{
    Q_OBJECT

public:
    explicit Worker(QSharedPtr<Task> task, QObject *parent = Q_NULLPTR)
        : QObject(parent)
        , m_task(task)
    {}

protected:
    void run() override
    {
        auto exitStatus = 0;
        // some code
        if (1 == exitStatus)
            m_task->setErrorText("Failed to access input file");
        // some oode
    }

private:
    QSharedPtr<Task> m_task;
};

My question is: do I really have to use locking when using QString in different threads and is the above example code correct?

Thanks.

[VRonin]

@Max-Logvinov said in Share QString between threads:

do I really have to use locking when using QString in different threads

Yes, not just for QString, it's a general rule to avoid race conditions

@Max-Logvinov said in Share QString between threads:

is the above example code correct

QWriteLocker locker(&m_lock);
        if (errorText != m_errorText)  {
            m_errorText = errorText;
            locker.unlock();
            emit errorTextChanged();
        }

is a bit redundant

        if (errorText != m_errorText)  {
            m_lock.lockForWrite();
            m_errorText = errorText;
            lm_lock.unlock();
            emit errorTextChanged();
        }

works already.

For the rest I cannot see anything too obviously wrong

[Max Logvinov]

@VRonin, thank you for your answer.

Yes, not just for QString

The C++11 Concurrency Library provides std::atomic, which I can use with any TriviallyCopyable type. But not with QString.

is a bit redundant

You right. In the example above, there is no need to lock before comparing strings. But let me clarify one thing: if there will be more than one thread, that can simultaneously write to the string, then my example above will not be redundant, isn't it?

[VRonin]

@Max-Logvinov said in Share QString between threads:

The C++11 Concurrency Library provides std::atomic, which I can use with any TriviallyCopyable type. But not with QString.

That's because QString is not trivially copyable, as are a lot of classes, see http://en.cppreference.com/w/cpp/types/is_trivially_copyable

@Max-Logvinov said in Share QString between threads:

hat can simultaneously write to the string, then my example above will not be redundant, isn't it?

You are correct, I missed it.

[Max Logvinov]

@VRonin, once again, thank you for your clear answer.

[kshegunov]

I have two remarks that aren't explicitly linked to the question.

1. Are you convinced you need to use a new thread for each task and then delete it. This isn't the best of ideas as on some OSes (like my lovely Linux) threads are pretty heavy to create/destroy. Perhaps you should consider moving to the worker object idiom or to using a thread pool.
2. If QSharedPtr is what I think, I strongly advise you not to shoot yourself in the foot by delegating responsibility for memory management to a black box. QObject doesn't work that well with shared pointers, as it has a special ownership (and organizational) model.
   Here are some thoughts and a little bit more from the mailing list. If you need to track lifetime of QObjects when you don't own them use QPointer, if you own them and want to be throw-safe use QScopedPointer (or its STL equivalent).

[Max Logvinov]

@kshegunov, thank you for your notes.

1. Of course, I'm using a thread pool and the example above is just for understanding content of my question.
2. Yes, it's a typo. I mean a QSharedPointer. But I really don't see any reasons to don't use QSharedPointer with a QObject if you do it wisely. Here is a part of my real code:

class Task : public QObject
{
    Q_OBJECT

    friend class TaskManager;

private:
    Task(const QString &inputFile);
    ~Task() {}

public:
    void setParent(QObject *parent) = delete;
};

typedef QSharedPointer<Task> TaskPtr;

class TaskManager : public QObject
{
    Q_OBJECT

public:
    qint64 addTask(const QString &inputFile)
    {
        static auto deleteTask = [] (Task *task) {
            delete task;
        };

        _tasks << TaskPtr(new Task(inputFile), deleteTask);
        emit taskAdded();
    }

signals:
    void taskAdded();

private:
    QList<TaskPtr> m_tasks;
};

As you can see, you can't set the parent for the Task. Also, instances of the Task is created/deleted only by the TaskManager. This looks good to me.

[kshegunov]

@Max-Logvinov said in Share QString between threads:

As you can see, you can't set the parent for the Task.

Are you sure about this? Suppose the following code:

QObject * coolParent;
QObject * task = new Task(inputFile);
task->setParent(coolParent);

Also, instances of the Task is created/deleted only by the TaskManager.

They are created by the TaskManager but there's no guarantee about deletion, or in which thread it happens. It all depends on who (which object) loses the last reference to the task and in which thread context this is happening.

This looks good to me.

Just read the mailing list threads. It's up to you in the end how to manage the memory, but you've been warned.

[Max Logvinov]

@kshegunov, thanks for your notes, I'll keep in mind.