StringTable: Ensure only one GC() thread is running at a time

The possible issue with the current implementation is that
in theory many possible GC() are being executed in parallel.
In this case just one of them is really working and others
are waiting on the locked mutex (first line of the GC()
method). In such a scenario when a call
to StringTablePrivate::insert() is being executed from one
more thread, it may happen that the working GC() thread
is stopped (since m_stopGCRequested.fetchAndStoreAcquire(true)
was executed from insert()) and later the mutex lock may be
granted to the other awaiting GC() thread instead to the
thread which executes insert() method. In this unlikely
scenario the GC() thread won't be canceled and the lock
inside the insert() method may be locked for considerable
amount of time, what is not desired.

The goal of this patch is to resolve the possible issue above
and to simplify the code by eliminating the m_stopGCRequested
variable and make use of QFuture.cancel() / QFuture.isCanceled()
API instead. In addition, since we control now only one
possible thread that executes the GC(), there is no need for
future synchonizer anymore.

GC() function can't be run in parallel in different threads,
as the whole body of GC() is protected with mutex. This means
that whenever a new scheduled call to GC() is being executed,
this new call waits on the mentioned mutex at the beginning of GC().

So, instead of protecting the whole body of GC() with a mutex,
we ensure that the old call to GC() is already finished (if not,
we also cancel the old call) while preparing an asynchronous
call to start a new GC() from inside startGC() method.

Whenever we are calling the insert() method, we still protect the access
to m_strings with a mutex (as insert() is designed to be called
from different threads in parallel). Just after locking the mutex
we are canceling any possible ongoing call to GC(). After canceling
the GC() call, we are sure that no new call to GC() will be executed
until we unlock the mutex, so it's safe now to modify the m_string
data.

Change-Id: If72d0a6f98fb414c6c63117bc9baa667d17e1ffe
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

1 files changed, 29 insertions, 31 deletions

diff --git a/src/plugins/cpptools/stringtable.cpp b/src/plugins/cpptools/stringtable.cpp
index 7f5d0178a3..331a56faa1 100644
--- a/src/plugins/cpptools/stringtable.cpp
+++ b/src/plugins/cpptools/stringtable.cpp
@@ -30,7 +30,6 @@
 
 #include <QDebug>
 #include <QElapsedTimer>
-#include <QFutureSynchronizer>
 #include <QMutex>
 #include <QSet>
 #include <QThreadPool>
@@ -50,35 +49,19 @@ class StringTablePrivate : public QObject
 {
 public:
     StringTablePrivate();
-    ~StringTablePrivate() override { m_futureSynchronizer.waitForFinished(); }
+    ~StringTablePrivate() override { cancelAndWait(); }
 
+    void cancelAndWait();
     QString insert(const QString &string);
-    void addFuture(const QFuture<void> &future);
-    void startGC() { addFuture(Utils::runAsync(&StringTablePrivate::GC, this)); }
-    void GC();
+    void startGC();
+    void GC(QFutureInterface<void> &futureInterface);
 
-    QFutureSynchronizer<void> m_futureSynchronizer;
-
-    mutable QMutex m_lock;
-    QAtomicInt m_stopGCRequested{false};
+    QFuture<void> m_future;
+    QMutex m_lock;
     QSet<QString> m_strings;
     QTimer m_gcCountDown;
 };
 
-void StringTablePrivate::addFuture(const QFuture<void> &future)
-{
-    m_futureSynchronizer.addFuture(future);
-    const QList<QFuture<void>> futures = m_futureSynchronizer.futures();
-    const int maxFuturesCount = 10;
-    if (futures.count() <= maxFuturesCount)
-        return;
-    m_futureSynchronizer.clearFutures();
-    for (const auto &future : futures) {
-        if (!future.isFinished())
-            m_futureSynchronizer.addFuture(future);
-    }
-}
-
 static StringTablePrivate *m_instance = nullptr;
 
 StringTablePrivate::StringTablePrivate()
@@ -96,6 +79,14 @@ QString StringTable::insert(const QString &string)
     return m_instance->insert(string);
 }
 
+void StringTablePrivate::cancelAndWait()
+{
+    if (!m_future.isRunning())
+        return;
+    m_future.cancel();
+    m_future.waitForFinished();
+}
+
 QString StringTablePrivate::insert(const QString &string)
 {
     if (string.isEmpty())
@@ -107,12 +98,21 @@ QString StringTablePrivate::insert(const QString &string)
 #endif
 #endif
 
-    m_stopGCRequested.fetchAndStoreAcquire(true);
+    QMutexLocker locker(&m_lock);
+    // From this point of time any possible new call to startGC() will be held until
+    // we finish this function. So we are sure that after canceling the running GC() method now,
+    // no new call to GC() will be executed until we finish this function.
+    cancelAndWait();
+    // A possibly running GC() thread already finished, so it's safe to modify m_strings from
+    // now until we unlock the mutex.
+    return *m_strings.insert(string);
+}
 
+void StringTablePrivate::startGC()
+{
     QMutexLocker locker(&m_lock);
-    QString result = *m_strings.insert(string);
-    m_stopGCRequested.fetchAndStoreRelease(false);
-    return result;
+    cancelAndWait();
+    m_future = Utils::runAsync(&StringTablePrivate::GC, this);
 }
 
 void StringTable::scheduleGC()
@@ -143,10 +143,8 @@ static inline bool isQStringInUse(const QString &string)
 #endif
 }
 
-void StringTablePrivate::GC()
+void StringTablePrivate::GC(QFutureInterface<void> &futureInterface)
 {
-    QMutexLocker locker(&m_lock);
-
     int initialSize = 0;
     QElapsedTimer timer;
     if (DebugStringTable) {
@@ -156,7 +154,7 @@ void StringTablePrivate::GC()
 
     // Collect all QStrings which have refcount 1. (One reference in m_strings and nowhere else.)
     for (QSet<QString>::iterator i = m_strings.begin(); i != m_strings.end();) {
-        if (m_stopGCRequested.testAndSetRelease(true, false))
+        if (futureInterface.isCanceled())
             return;
 
         if (!isQStringInUse(*i))