QFileInfoGatherer: fix race conditions pt.1: abort

Fix a race on the 'abort' variable. While there was a mutex lock around
the code that sets the variable in ~QFileInfoGatherer, there was no
protection in getFileInfos(), where it is read:

   // T:this->thread()  // T:*this
                        // in getFileInfos(), after last mutex.unlock()
   mutex.lock();
   abort = true;        while (!abort...
   // ...               // ...

Fix by making 'abort' an atomic. This means that we can drop the mutex
locker in the destructor, too. We still mostly access 'abort' under
protection of the mutex, because we need to protect other variables that
just happen to be accessed together with 'abort', but we avoid the mutex
lock/unlock on each iteration of the while loop in getFileInfos().

Also cleaned up the logic in run():

- by using the canonical form of condition.wait() (in a loop that
  checks the condition), we can ensure that !path.isEmpty() and avoid
  having to use the updateFiles boolean.
- by checking for abort.load() after we return from
  condition.wait(), we minimise the waiting time for thread
  exit.
- by using different local names, we avoid having to this->qualify members.

Also changed one condition.wakeOne() to wakeAll() for consistency
with fetchExtendedInformation().

Change-Id: If35f338fe774546616ec287c1c37e2c32ed05f1a
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: David Faure <faure@kde.org>
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>

diff --git a/src/widgets/dialogs/qfileinfogatherer.cpp b/src/widgets/dialogs/qfileinfogatherer.cpp
index 0393565..6d9c0ca 100644 (file)
--- a/src/widgets/dialogs/qfileinfogatherer.cpp
+++ b/src/widgets/dialogs/qfileinfogatherer.cpp
@@ -96,10 +96,8 @@ QFileInfoGatherer::QFileInfoGatherer(QObject *parent)
 */
 QFileInfoGatherer::~QFileInfoGatherer()
 {
-    QMutexLocker locker(&mutex);
-    abort = true;
-    condition.wakeOne();
-    locker.unlock();
+    abort.store(true);
+    condition.wakeAll();
     wait();
 }
 
@@ -204,25 +202,18 @@ void QFileInfoGatherer::list(const QString &directoryPath)
 void QFileInfoGatherer::run()
 {
     forever {
-        bool updateFiles = false;
         QMutexLocker locker(&mutex);
-        if (abort) {
-            return;
-        }
-        if (this->path.isEmpty())
+        while (!abort.load() && path.isEmpty())
             condition.wait(&mutex);
-        QString path;
-        QStringList list;
-        if (!this->path.isEmpty()) {
-            path = this->path.first();
-            list = this->files.first();
-            this->path.pop_front();
-            this->files.pop_front();
-            updateFiles = true;
-        }
+        if (abort.load())
+            return;
+        const QString thisPath = path.front();
+        path.pop_front();
+        const QStringList thisList = files.front();
+        files.pop_front();
         locker.unlock();
-        if (updateFiles)
-            getFileInfos(path, list);
+
+        getFileInfos(thisPath, thisList);
     }
 }
 
@@ -316,7 +307,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
     QString itPath = QDir::fromNativeSeparators(files.isEmpty() ? path : QLatin1String(""));
     QDirIterator dirIt(itPath, QDir::AllEntries | QDir::System | QDir::Hidden);
     QStringList allFiles;
-    while(!abort && dirIt.hasNext()) {
+    while (!abort.load() && dirIt.hasNext()) {
         dirIt.next();
         fileInfo = dirIt.fileInfo();
         allFiles.append(fileInfo.fileName());
@@ -326,7 +317,7 @@ void QFileInfoGatherer::getFileInfos(const QString &path, const QStringList &fil
         emit newListOfFiles(path, allFiles);
 
     QStringList::const_iterator filesIt = filesToCheck.constBegin();
-    while(!abort && filesIt != filesToCheck.constEnd()) {
+    while (!abort.load() && filesIt != filesToCheck.constEnd()) {
         fileInfo.setFile(path + QDir::separator() + *filesIt);
         ++filesIt;
         fetch(fileInfo, base, firstTime, updatedFiles, path);

diff --git a/src/widgets/dialogs/qfileinfogatherer_p.h b/src/widgets/dialogs/qfileinfogatherer_p.h
index c07c908..cc03e99 100644 (file)
--- a/src/widgets/dialogs/qfileinfogatherer_p.h
+++ b/src/widgets/dialogs/qfileinfogatherer_p.h
@@ -181,9 +181,9 @@ private:
     void fetch(const QFileInfo &info, QElapsedTimer &base, bool &firstTime, QList<QPair<QString, QFileInfo> > &updatedFiles, const QString &path);
     QString translateDriveName(const QFileInfo &drive) const;
 
-    QMutex mutex;
+    mutable QMutex mutex;
     QWaitCondition condition;
-    volatile bool abort;
+    QAtomicInt abort;
 
     QStack<QString> path;
     QStack<QStringList> files;