Prevent concurrent access from different threads?

[agarny]

Hi, I was wondering whether I could have some views on the following.

My application uses a thread to run a simulation and generate simulation data. A thread is used so that my application's GUI can remain responsive. Now, while a simulation is being run, my application's main thread makes successive calls to QTimer::singleShot() to fetch any new simulation data that has been generated, as well as to plot whatever the user wants to visualise. The user can decide on what to visualise by creating/removing traces, this through my application's GUI.

If I pre-select some traces and run a simulation, then everything works as expected. However, if, during a simulation, I remove a trace, then my application may crash. This is because the fetching function I call from QTimer::singleShot() goes through all the different traces and update them. Now, if a trace has been removed during a simulation and, more importantly, in the middle of my fetching function, then my fetching function might try to use a trace which doesn't exist anymore, hence the crash.

So, what would you do to avoid this problem? I originally thought I could solve this problem by using a mutex, but that failed and I believe this might be because of my fetching function being called through QTimer::singleShot() and therefore running in its own thread?

Anyway, any idea/help on the above would be much appreciated...

Cheers, Alan.

[amleto]

mutexes/critical sections stop concurrent access from different threads.

without seeing your code (read my sig), we can't say much more. It all depends how things go wrong when X uses Y that doesn't exist.

You should ask yourself: If Y doesn't exist, why & how can X use it?

Simply, you need a thread safe mechanism to remove traces - you will need to...

Analyse where it is safe to do this (the range of the critical section(s)).
Implement a mechanism (probably using signals/slots) that sends intention to remove traces from gui to simulation.

[wysota]

because of my fetching function being called through QTimer::singleShot() and therefore running in its own thread?

Timers do not start any new threads.

If using a mutex fails then apparently you're protecting the wrong resource or you're not protecting any resources at all.

[agarny]

amleto, I already use signals/slots to add/remove a trace (since it's done through my application's GUI). Anyway, you asked for some code, but it's not always easy to provide some but I have tried below...

wysota, thanks, I wasn't sure about QTimer::singleShot() creating a new thread or not. I should have checked Qt's code and well... now, I have...

My .cpp and .h files are as follows (to make it 'short', I removed comments and, I know, what I believe to be irrelevant code):

class SingleCellSimulationViewWidget : public Core::ViewWidget
{
    Q_OBJECT
 
public:
    ...
 
private:
    ...
 
    QMutex mSimulationMutex;
    QMutex mTracesMutex;
 
    void updateResults(SingleCellSimulationViewSimulation *pSimulation,
                       const qulonglong &pSize,
                       const bool &pReplot = false);
 
    void checkResults(SingleCellSimulationViewSimulation *pSimulation);
 
    ...
 
private Q_SLOTS:
    ...
 
    void resetFileTabIcon();
 
    void showHideParameterPlot(const QString &pFileName,
                               CellMLSupport::CellmlFileRuntimeModelParameter *pParameter,
                               const bool &pShowParameterPlot);
 
    void callCheckResults();
 
    ...
};

class SingleCellSimulationViewWidget : public Core::ViewWidget
{
    Q_OBJECT

public:
    ...

private:
    ...

    QMutex mSimulationMutex;
    QMutex mTracesMutex;

    void updateResults(SingleCellSimulationViewSimulation *pSimulation,
                       const qulonglong &pSize,
                       const bool &pReplot = false);

    void checkResults(SingleCellSimulationViewSimulation *pSimulation);

    ...

private Q_SLOTS:
    ...

    void resetFileTabIcon();

    void showHideParameterPlot(const QString &pFileName,
                               CellMLSupport::CellmlFileRuntimeModelParameter *pParameter,
                               const bool &pShowParameterPlot);

    void callCheckResults();

    ...
};

To copy to clipboard, switch view to plain text mode 

void SingleCellSimulationViewWidget::resetFileTabIcon()
{
    SingleCellSimulationViewSimulation *simulation;
 
    {
        QMutexLocker mutexLocker(&mSimulationMutex);
 
        simulation = mStoppedSimulations.first();
 
        mStoppedSimulations.removeFirst();
    }
 
    emit updateFileTabIcon(simulation->fileName(), QIcon());
}
 
void SingleCellSimulationViewWidget::showHideParameterPlot(const QString &pFileName,
                                                           CellMLSupport::CellmlFileRuntimeModelParameter *pParameter,
                                                           const bool &pShowParameterPlot)
{
    QString key = parameterKey(pFileName, pParameter);
 
    QMutexLocker mutexLocker(&mTracesMutex);
    QwtPlotCurve *trace = mTraces.value(key);
 
    if (trace && !pShowParameterPlot) {
        mActiveGraphPanel->plot()->removeTrace(trace);
 
        mTraces.remove(key);
    } else if (!trace && pShowParameterPlot) {
        SingleCellSimulationViewSimulationResults *results = mSimulation->results();
 
        double *yData;
 
        ...
 
        QwtPlotCurve *trace = mActiveGraphPanel->plot()->addTrace(results->points(), yData, results->size());
 
        mTraces.insert(key, trace);
    }
}
 
void SingleCellSimulationViewWidget::updateResults(SingleCellSimulationViewSimulation *pSimulation,
                                                   const qulonglong &pSize,
                                                   const bool &pReplot)
{
    SingleCellSimulationViewSimulation *simulation = pSimulation?pSimulation:mSimulation;
 
    if (simulation == mSimulation) {
        QMutexLocker mutexLocker(&mTracesMutex);
 
        QMap<QString, QwtPlotCurve *>::const_iterator iter = mTraces.constBegin();
 
        while (iter != mTraces.constEnd()) {
            QString fileName = iter.key();
 
            ...
 
            QwtPlotCurve *trace = iter.value();
 
            if (!fileName.compare(mSimulation->fileName())) {
                double *yData;
 
                ...
 
                qulonglong oldSize = trace->dataSize();
 
                trace->setRawSamples(simulation->results()->points(), yData, pSize);
 
                if (!pReplot && (pSize > 1))
                    qobject_cast<SingleCellSimulationViewGraphPanelPlotWidget *>(trace->plot())->drawTraceSegment(trace, oldSize?oldSize-1:0, pSize-1);
            }
 
            ++iter;
        }
 
        ...
    } else {
        ...
    }
}
 
void SingleCellSimulationViewWidget::checkResults(SingleCellSimulationViewSimulation *pSimulation)
{
    qulonglong simulationResultsSize = pSimulation->results()->size();
 
    if (simulationResultsSize != mOldSimulationResultsSizes.value(pSimulation)) {
        mOldSimulationResultsSizes.insert(pSimulation, simulationResultsSize);
 
        updateResults(pSimulation, simulationResultsSize);
    }
 
    if (pSimulation->isRunning()) {
        mCheckResultsSimulations << pSimulation;
 
        QTimer::singleShot(0, this, SLOT(callCheckResults()));
    }
}
 
void SingleCellSimulationViewWidget::callCheckResults()
{
    SingleCellSimulationViewSimulation *simulation;
 
    {
        QMutexLocker mutexLocker(&mSimulationMutex);
 
        simulation = mCheckResultsSimulations.first();
 
        mCheckResultsSimulations.removeFirst();
    }
 
    checkResults(simulation);
}

void SingleCellSimulationViewWidget::resetFileTabIcon()
{
    SingleCellSimulationViewSimulation *simulation;

    {
        QMutexLocker mutexLocker(&mSimulationMutex);

        simulation = mStoppedSimulations.first();

        mStoppedSimulations.removeFirst();
    }

    emit updateFileTabIcon(simulation->fileName(), QIcon());
}

void SingleCellSimulationViewWidget::showHideParameterPlot(const QString &pFileName,
                                                           CellMLSupport::CellmlFileRuntimeModelParameter *pParameter,
                                                           const bool &pShowParameterPlot)
{
    QString key = parameterKey(pFileName, pParameter);

    QMutexLocker mutexLocker(&mTracesMutex);
    QwtPlotCurve *trace = mTraces.value(key);

    if (trace && !pShowParameterPlot) {
        mActiveGraphPanel->plot()->removeTrace(trace);

        mTraces.remove(key);
    } else if (!trace && pShowParameterPlot) {
        SingleCellSimulationViewSimulationResults *results = mSimulation->results();

        double *yData;

        ...

        QwtPlotCurve *trace = mActiveGraphPanel->plot()->addTrace(results->points(), yData, results->size());

        mTraces.insert(key, trace);
    }
}

void SingleCellSimulationViewWidget::updateResults(SingleCellSimulationViewSimulation *pSimulation,
                                                   const qulonglong &pSize,
                                                   const bool &pReplot)
{
    SingleCellSimulationViewSimulation *simulation = pSimulation?pSimulation:mSimulation;

    if (simulation == mSimulation) {
        QMutexLocker mutexLocker(&mTracesMutex);

        QMap<QString, QwtPlotCurve *>::const_iterator iter = mTraces.constBegin();

        while (iter != mTraces.constEnd()) {
            QString fileName = iter.key();

            ...

            QwtPlotCurve *trace = iter.value();

            if (!fileName.compare(mSimulation->fileName())) {
                double *yData;

                ...

                qulonglong oldSize = trace->dataSize();

                trace->setRawSamples(simulation->results()->points(), yData, pSize);

                if (!pReplot && (pSize > 1))
                    qobject_cast<SingleCellSimulationViewGraphPanelPlotWidget *>(trace->plot())->drawTraceSegment(trace, oldSize?oldSize-1:0, pSize-1);
            }

            ++iter;
        }

        ...
    } else {
        ...
    }
}

void SingleCellSimulationViewWidget::checkResults(SingleCellSimulationViewSimulation *pSimulation)
{
    qulonglong simulationResultsSize = pSimulation->results()->size();

    if (simulationResultsSize != mOldSimulationResultsSizes.value(pSimulation)) {
        mOldSimulationResultsSizes.insert(pSimulation, simulationResultsSize);

        updateResults(pSimulation, simulationResultsSize);
    }

    if (pSimulation->isRunning()) {
        mCheckResultsSimulations << pSimulation;

        QTimer::singleShot(0, this, SLOT(callCheckResults()));
    }
}

void SingleCellSimulationViewWidget::callCheckResults()
{
    SingleCellSimulationViewSimulation *simulation;

    {
        QMutexLocker mutexLocker(&mSimulationMutex);

        simulation = mCheckResultsSimulations.first();

        mCheckResultsSimulations.removeFirst();
    }

    checkResults(simulation);
}

To copy to clipboard, switch view to plain text mode 

[wysota]

And where are the threads involved here? I see a couple of problems with your code related to threading but first I'd like to know if we do have any multithreading here at all.

[agarny]

And where are the threads involved here? I see a couple of problems with your code related to threading but first I'd like to know if we do have any multithreading here at all.

Exactly, the mutexes I use in the code I gave are all in functions called from the same thread and therefore unnecessary. I have just had a chat with someone and to cut a long story short, I needed to learn more about Qt's event loop, and now that I know a bit more about it, I can see that my mutexes were unnecessary and the fact that my application crashes is most likely due to some non-reentrant / non-atomic functions, from two different threads, which access some shared data for reading and/or writing purposes. So, yes, I am still very much learning (the hard way) about threading...