From fddeec60cba806b88c5eb0e4758c3951457518a2 Mon Sep 17 00:00:00 2001
From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
Date: Thu, 15 Dec 2022 01:06:43 +0100
Subject: [PATCH] Wait conditions example: fix an incorrect condition variable
 usage
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

3a449bbb69c9a3c3a5bc6a052f2de98ab79be7e9 amended the code to remove
acquiring a lock when waking up a condition variable. It is fine to
not have a lock associated when waking a condition variable; what I
misunderstood was the scope of the lock, which (and this underlines
the importance of commenting _what exactly_ a lock protects, for
each and ever lock) protected both the buffer as well as the counter
of the buffer. This made my reasoning flawed: it is necessary to keep
the lock while notifying, otherwise the counterpart could verify the
condition isn't satisfied and wait (e.g. see numUsedBytes==0), missing
the wake from the other thread (which could arrive between the check and
the wait).

Amends the previous commit.

Change-Id: If7db2d045331f1b33b976fb6bf6aa9117c41678f
Pick-to: 5.15 6.2 6.4 6.5
Reviewed-by: Morten Johan Sørvig <morten.sorvig@qt.io>
---
 .../threads/doc/src/waitconditions.qdoc       |  7 ++----
 .../threads/waitconditions/waitconditions.cpp | 24 +++++++++++--------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/examples/corelib/threads/doc/src/waitconditions.qdoc b/examples/corelib/threads/doc/src/waitconditions.qdoc
index 7715bd8ab63..ae9e767c138 100644
--- a/examples/corelib/threads/doc/src/waitconditions.qdoc
+++ b/examples/corelib/threads/doc/src/waitconditions.qdoc
@@ -75,16 +75,13 @@
     that the condition \c bufferNotEmpty is true, since \c
     numUsedBytes is necessarily greater than 0.
 
-    The QWaitCondition::wait() function accepts a
+    We guard all accesses to the \c numUsedBytes variable with a
+    mutex. In addition, the QWaitCondition::wait() function accepts a
     mutex as its argument. This mutex is unlocked before the thread
     is put to sleep and locked when the thread wakes up. Furthermore,
     the transition from the locked state to the wait state is atomic,
     to prevent race conditions from occurring.
 
-    Accesses to the \c numUsedBytes variable do not need mutex
-    protection, as that variable is a QAtomicInt; atomic variables
-    do not participate in data races.
-
     \section1 Consumer Class
 
     Let's turn to the \c Consumer class:
diff --git a/examples/corelib/threads/waitconditions/waitconditions.cpp b/examples/corelib/threads/waitconditions/waitconditions.cpp
index 36f02f78968..bdc24acd8c2 100644
--- a/examples/corelib/threads/waitconditions/waitconditions.cpp
+++ b/examples/corelib/threads/waitconditions/waitconditions.cpp
@@ -2,7 +2,6 @@
 // Copyright (C) 2022 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com, author Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
 // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause
 
-#include <QAtomicInt>
 #include <QCoreApplication>
 #include <QMutex>
 #include <QMutexLocker>
@@ -18,13 +17,12 @@
 constexpr int DataSize = 100000;
 constexpr int BufferSize = 8192;
 
-QMutex mutex; // protects the buffer
+QMutex mutex; // protects the buffer and the counter
 char buffer[BufferSize];
+int numUsedBytes;
 
 QWaitCondition bufferNotEmpty;
 QWaitCondition bufferNotFull;
-
-QAtomicInt numUsedBytes;
 //! [0]
 
 //! [1]
@@ -43,14 +41,17 @@ private:
         for (int i = 0; i < DataSize; ++i) {
             {
                 const QMutexLocker locker(&mutex);
-                while (numUsedBytes.loadAcquire() == BufferSize)
+                while (numUsedBytes == BufferSize)
                     bufferNotFull.wait(&mutex);
             }
 
             buffer[i % BufferSize] = "ACGT"[QRandomGenerator::global()->bounded(4)];
 
-            numUsedBytes.fetchAndAddRelease(1);
-            bufferNotEmpty.wakeAll();
+            {
+                const QMutexLocker locker(&mutex);
+                ++numUsedBytes;
+                bufferNotEmpty.wakeAll();
+            }
         }
     }
 };
@@ -72,14 +73,17 @@ private:
         for (int i = 0; i < DataSize; ++i) {
             {
                 const QMutexLocker locker(&mutex);
-                while (numUsedBytes.loadAcquire() == 0)
+                while (numUsedBytes == 0)
                     bufferNotEmpty.wait(&mutex);
             }
 
             fprintf(stderr, "%c", buffer[i % BufferSize]);
 
-            numUsedBytes.fetchAndAddRelease(-1);
-            bufferNotFull.wakeAll();
+            {
+                const QMutexLocker locker(&mutex);
+                --numUsedBytes;
+                bufferNotFull.wakeAll();
+            }
         }
         fprintf(stderr, "\n");
     }