From 6660c365b99d10037c9a32b5abc048ca98cbbc25 Mon Sep 17 00:00:00 2001
From: Jeremy Ong <jcong@amazon.com>
Date: Thu, 23 Dec 2021 16:25:44 -0700
Subject: [PATCH] Disambiguate null interface case

In some cases, the interface being queried may be persistently nullptr
(e.g. a gem isn't active, or a feature was disabled by a cvar). In this
case, querying the interface would always enter the writer-lock branch
with existing code because nullptr was synonmous with "we haven't
queried for this variable yet."

This PR adds an additional state variable that is set when s_instance is
assigned to, so that after the variable is queried and found to be null,
future queries can enter the read-lock branch.

Signed-off-by: Jeremy Ong <jcong@amazon.com>
---
 .../Framework/AzCore/AzCore/Interface/Interface.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/Code/Framework/AzCore/AzCore/Interface/Interface.h b/Code/Framework/AzCore/AzCore/Interface/Interface.h
index 8664e2905f..8f72cfa109 100644
--- a/Code/Framework/AzCore/AzCore/Interface/Interface.h
+++ b/Code/Framework/AzCore/AzCore/Interface/Interface.h
@@ -109,6 +109,7 @@ namespace AZ
          */
         static EnvironmentVariable<T*> s_instance;
         static AZStd::shared_mutex s_mutex;
+        static bool s_instanceAssigned;
     };
 
     template <typename T>
@@ -117,6 +118,9 @@ namespace AZ
     template <typename T>
     AZStd::shared_mutex Interface<T>::s_mutex;
 
+    template <typename T>
+    bool Interface<T>::s_instanceAssigned;
+
     template <typename T>
     void Interface<T>::Register(T* type)
     {
@@ -135,18 +139,19 @@ namespace AZ
         AZStd::unique_lock<AZStd::shared_mutex> lock(s_mutex);
         s_instance = Environment::CreateVariable<T*>(GetVariableName());
         s_instance.Get() = type;
+        s_instanceAssigned = true;
     }
 
     template <typename T>
     void Interface<T>::Unregister(T* type)
     {
-        if (!s_instance || !s_instance.Get())
+        if (!s_instanceAssigned)
         {
             AZ_Assert(false, "Interface '%s' not registered on this module!", AzTypeInfo<T>::Name());
             return;
         }
 
-        if (s_instance.Get() != type)
+        if (s_instance && s_instance.Get() != type)
         {
             AZ_Assert(false, "Interface '%s' is not the same instance that was registered! [Expected '%p', Found '%p']", AzTypeInfo<T>::Name(), type, s_instance.Get());
             return;
@@ -156,6 +161,7 @@ namespace AZ
         AZStd::unique_lock<AZStd::shared_mutex> lock(s_mutex);
         *s_instance = nullptr;
         s_instance.Reset();
+        s_instanceAssigned = false;
     }
 
     template <typename T>
@@ -165,9 +171,9 @@ namespace AZ
         // This is the fast path which won't block.
         {
             AZStd::shared_lock<AZStd::shared_mutex> lock(s_mutex);
-            if (s_instance)
+            if (s_instanceAssigned)
             {
-                return s_instance.Get();
+                return s_instance ? s_instance.Get() : nullptr;
             }
         }
 
@@ -175,6 +181,7 @@ namespace AZ
         // take the full lock and request it.
         AZStd::unique_lock<AZStd::shared_mutex> lock(s_mutex);
         s_instance = Environment::FindVariable<T*>(GetVariableName());
+        s_instanceAssigned = true;
         return s_instance ? s_instance.Get() : nullptr;
     }