From 4b57909fb0d0d160ccbf2c06789e09c0dd28b141 Mon Sep 17 00:00:00 2001
From: reuk <reuk@users.noreply.github.com>
Date: Mon, 4 Oct 2021 15:35:20 +0100
Subject: [PATCH] Singleton:  Fix thread sanitizer warning about race on
 "instance" data member

---
 modules/juce_core/juce_core.h             |  2 +-
 modules/juce_core/memory/juce_Singleton.h | 90 +++++++++++------------
 2 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/modules/juce_core/juce_core.h b/modules/juce_core/juce_core.h
index 3302d93231..ccbf6b1c55 100644
--- a/modules/juce_core/juce_core.h
+++ b/modules/juce_core/juce_core.h
@@ -244,6 +244,7 @@ JUCE_END_IGNORE_WARNINGS_MSVC
 #include "memory/juce_ReferenceCountedObject.h"
 #include "memory/juce_ScopedPointer.h"
 #include "memory/juce_OptionalScopedPointer.h"
+#include "containers/juce_ScopedValueSetter.h"
 #include "memory/juce_Singleton.h"
 #include "memory/juce_WeakReference.h"
 #include "threads/juce_ScopedLock.h"
@@ -259,7 +260,6 @@ JUCE_END_IGNORE_WARNINGS_MSVC
 #include "containers/juce_ListenerList.h"
 #include "containers/juce_OwnedArray.h"
 #include "containers/juce_ReferenceCountedArray.h"
-#include "containers/juce_ScopedValueSetter.h"
 #include "containers/juce_SortedSet.h"
 #include "containers/juce_SparseSet.h"
 #include "containers/juce_AbstractFifo.h"
diff --git a/modules/juce_core/memory/juce_Singleton.h b/modules/juce_core/memory/juce_Singleton.h
index 8419c37b0b..629e856842 100644
--- a/modules/juce_core/memory/juce_Singleton.h
+++ b/modules/juce_core/memory/juce_Singleton.h
@@ -47,53 +47,49 @@ struct SingletonHolder  : private MutexType // (inherited so we can use the empt
            If you're having trouble cleaning up your singletons, perhaps consider using the
            SharedResourcePointer class instead.
         */
-        jassert (instance == nullptr);
+        jassert (instance.load() == nullptr);
     }
 
     /** Returns the current instance, or creates a new instance if there isn't one. */
     Type* get()
     {
-        if (instance == nullptr)
+        if (auto* ptr = instance.load())
+            return ptr;
+
+        typename MutexType::ScopedLockType sl (*this);
+
+        if (auto* ptr = instance.load())
+            return ptr;
+
+        auto once = onlyCreateOncePerRun; // (local copy avoids VS compiler warning about this being constant)
+
+        if (once)
         {
-            typename MutexType::ScopedLockType sl (*this);
+            static bool createdOnceAlready = false;
 
-            if (instance == nullptr)
+            if (createdOnceAlready)
             {
-                auto once = onlyCreateOncePerRun; // (local copy avoids VS compiler warning about this being constant)
-
-                if (once)
-                {
-                    static bool createdOnceAlready = false;
-
-                    if (createdOnceAlready)
-                    {
-                        // This means that the doNotRecreateAfterDeletion flag was set
-                        // and you tried to create the singleton more than once.
-                        jassertfalse;
-                        return nullptr;
-                    }
-
-                    createdOnceAlready = true;
-                }
-
-                static bool alreadyInside = false;
-
-                if (alreadyInside)
-                {
-                    // This means that your object's constructor has done something which has
-                    // ended up causing a recursive loop of singleton creation..
-                    jassertfalse;
-                }
-                else
-                {
-                    alreadyInside = true;
-                    getWithoutChecking();
-                    alreadyInside = false;
-                }
+                // This means that the doNotRecreateAfterDeletion flag was set
+                // and you tried to create the singleton more than once.
+                jassertfalse;
+                return nullptr;
             }
+
+            createdOnceAlready = true;
         }
 
-        return instance;
+        static bool alreadyInside = false;
+
+        if (alreadyInside)
+        {
+            // This means that your object's constructor has done something which has
+            // ended up causing a recursive loop of singleton creation.
+            jassertfalse;
+            return nullptr;
+        }
+
+        const ScopedValueSetter<bool> scope (alreadyInside, true);
+        return getWithoutChecking();
     }
 
     /** Returns the current instance, or creates a new instance if there isn't one, but doesn't do
@@ -101,32 +97,30 @@ struct SingletonHolder  : private MutexType // (inherited so we can use the empt
     */
     Type* getWithoutChecking()
     {
-        if (instance == nullptr)
-        {
-            auto newObject = new Type(); // (create into a local so that instance is still null during construction)
-            instance = newObject;
-        }
+        if (auto* p = instance.load())
+            return p;
 
-        return instance;
+        auto* newObject = new Type(); // (create into a local so that instance is still null during construction)
+        instance.store (newObject);
+        return newObject;
     }
 
     /** Deletes and resets the current instance, if there is one. */
     void deleteInstance()
     {
         typename MutexType::ScopedLockType sl (*this);
-        auto old = instance;
-        instance = nullptr;
-        delete old;
+        delete instance.exchange (nullptr);
     }
 
     /** Called by the class's destructor to clear the pointer if it is currently set to the given object. */
     void clear (Type* expectedObject) noexcept
     {
-        if (instance == expectedObject)
-            instance = nullptr;
+        instance.compare_exchange_strong (expectedObject, nullptr);
     }
 
-    Type* instance = nullptr;
+    // This must be atomic, otherwise a late call to get() may attempt to read instance while it is
+    // being modified by the very first call to get().
+    std::atomic<Type*> instance { nullptr };
 };