birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Settings registry notification deadlock fix (#3065)

Browse Source 
* Added a StealHandlers function to AZ Event

The StealHandlers function is able to take all the handlers from an AZ
Event parameter and register them with the current AZ Event

This allows stealing handlers from expiring AZ Events, which is useful
for a lock and swap algorithm for thread safety.
1. Lock persistent AZ::Event
2. Swap persistent AZ::Event with local AZ::Event
3. Unlock persistent AZ::Event - Other threads can now add to this
   AZ::Event
4. Invoke handlers from local AZ::Event
5. Relock persistent AZ::Event
5. Swap local AZ::Event with persistent AZ::Event
6. Local AZ::Event now contains handlers that were added when the lock
   was free
7. Persistent AZ::Event now steals from local AZ::Event
8. Unlock persistent AZ::Event

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

* Separated SettingRegistry update/query mutex from Notifier update mutex

The Settings Registry update/query mutex is also better scoped to reduce
the amount of lock time.

The Notifier mutex being separate allows the Settings Registry to signal
a notification event without being under any mutex, by locking and
swapping the notifier event with a local instance

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

* Renamed StealHandlers function to ClaimHandlers
Replaced decltype keywords in ClaimHandlers to auto

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
This commit is contained in:
lumberyard-employee-dm
2021-08-13 16:20:08 -05:00
committed by GitHub
parent 63a78b906a
commit 9060423592
5 changed files with 151 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/EBus/Event.h
+6
View File
@@ -58,6 +58,12 @@ namespace AZ
Event& operator=(Event&& rhs);
//! Take the handlers registered with the other event
//! and move them to this event. The other will event
//! will be cleared after call
//! @param other event to move handlers
Event& ClaimHandlers(Event&& other);
//! Returns true if at least one handler is connected to this event.
bool HasHandlerConnected() const;
Code/Framework/AzCore/AzCore/EBus/Event.inl
+26
View File
@@ -207,6 +207,32 @@ namespace AZ
}
template <typename... Params>
auto Event<Params...>::ClaimHandlers(Event&& other) -> Event&
{
auto handlers = AZStd::move(other.m_handlers);
auto addList = AZStd::move(other.m_addList);
other.m_freeList = {};
other.m_updating = false;
AZStd::array handlerContainers{ &handlers, &addList };
for (AZStd::vector<Handler*>* handlerList : handlerContainers)
{
for (Handler* handler : *handlerList)
{
if (handler != nullptr)
{
handler->m_index = 0;
handler->m_event = this;
Connect(*handler);
}
}
}
return *this;
}
template <typename... Params>
bool Event<Params...>::HasHandlerConnected() const
{
Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp
+84 -27
View File
@@ -20,7 +20,7 @@
namespace AZ
{
template<typename T>
bool SettingsRegistryImpl::SetValueInternal(AZStd::string_view path, T value, SettingsRegistryInterface::Type type)
bool SettingsRegistryImpl::SetValueInternal(AZStd::string_view path, T value)
{
if (path.empty())
{
@@ -56,7 +56,6 @@ namespace AZ
static_assert(!AZStd::is_same_v<T, T>, "SettingsRegistryImpl::SetValueInternal called with unsupported type.");
}
m_notifiers.Signal(path, type);
return true;
}
return false;
@@ -157,11 +156,11 @@ namespace AZ
// Setting to empty string to prevent assert
path = "";
}
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Pointer pointer(path.data(), path.length());
if (pointer.IsValid())
{
AZStd::scoped_lock lock(m_settingMutex);
const rapidjson::Value* value = pointer.Get(m_settings);
if (value)
{
@@ -207,7 +206,7 @@ namespace AZ
{
NotifyEventHandler notifyHandler{ callback };
{
AZStd::scoped_lock lock(m_settingMutex);
AZStd::scoped_lock lock(m_notifierMutex);
notifyHandler.Connect(m_notifiers);
}
return notifyHandler;
@@ -217,7 +216,7 @@ namespace AZ
{
NotifyEventHandler notifyHandler{ AZStd::move(callback) };
{
AZStd::scoped_lock lock(m_settingMutex);
AZStd::scoped_lock lock(m_notifierMutex);
notifyHandler.Connect(m_notifiers);
}
return notifyHandler;
@@ -225,10 +224,35 @@ namespace AZ
void SettingsRegistryImpl::ClearNotifiers()
{
AZStd::scoped_lock lock(m_settingMutex);
AZStd::scoped_lock lock(m_notifierMutex);
m_notifiers.DisconnectAllHandlers();
}
void SettingsRegistryImpl::SignalNotifier(AZStd::string_view jsonPath, Type type)
{
// Move the Notifier AZ::Event to a local AZ::Event in order to allow
// the notifier handlers to be signaled outside of the notifier mutex
// This allows other threads to register notifiers while this thread
// is invoking the handlers
decltype(m_notifiers) localNotifierEvent;
{
AZStd::scoped_lock lock(m_notifierMutex);
localNotifierEvent = AZStd::move(m_notifiers);
}
localNotifierEvent.Signal(jsonPath, type);
{
// Swap the local handlers with the current m_notifiers which
// will contain any handlers added during the signaling of the
// local event
AZStd::scoped_lock lock(m_notifierMutex);
AZStd::swap(m_notifiers, localNotifierEvent);
// Append any added handlers to the m_notifier structure
m_notifiers.ClaimHandlers(AZStd::move(localNotifierEvent));
}
}
SettingsRegistryInterface::Type SettingsRegistryImpl::GetType(AZStd::string_view path) const
{
if (path.empty())
@@ -239,11 +263,11 @@ namespace AZ
path = "";
}
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Pointer pointer(path.data(), path.length());
if (pointer.IsValid())
{
AZStd::scoped_lock lock(m_settingMutex);
const rapidjson::Value* value = pointer.Get(m_settings);
if (value)
{
@@ -316,11 +340,11 @@ namespace AZ
// Setting to empty string to prevent assert
path = "";
}
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Pointer pointer(path.data(), path.length());
if (pointer.IsValid())
{
AZStd::scoped_lock lock(m_settingMutex);
const rapidjson::Value* value = pointer.Get(m_settings);
if (value)
{
@@ -333,32 +357,52 @@ namespace AZ
bool SettingsRegistryImpl::Set(AZStd::string_view path, bool value)
{
AZStd::scoped_lock lock(m_settingMutex);
return SetValueInternal(path, value, Type::Boolean);
if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
{
return false;
}
SignalNotifier(path, Type::Boolean);
return true;
}
bool SettingsRegistryImpl::Set(AZStd::string_view path, s64 value)
{
AZStd::scoped_lock lock(m_settingMutex);
return SetValueInternal(path, value, Type::Integer);
if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
{
return false;
}
SignalNotifier(path, Type::Integer);
return true;
}
bool SettingsRegistryImpl::Set(AZStd::string_view path, u64 value)
{
AZStd::scoped_lock lock(m_settingMutex);
return SetValueInternal(path, value, Type::Integer);
if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
{
return false;
}
SignalNotifier(path, Type::Integer);
return true;
}
bool SettingsRegistryImpl::Set(AZStd::string_view path, double value)
{
AZStd::scoped_lock lock(m_settingMutex);
return SetValueInternal(path, value, Type::FloatingPoint);
if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
{
return false;
}
SignalNotifier(path, Type::FloatingPoint);
return true;
}
bool SettingsRegistryImpl::Set(AZStd::string_view path, AZStd::string_view value)
{
AZStd::scoped_lock lock(m_settingMutex);
return SetValueInternal(path, value, Type::String);
if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
{
return false;
}
SignalNotifier(path, Type::String);
return true;
}
bool SettingsRegistryImpl::Set(AZStd::string_view path, const char* value)
@@ -376,7 +420,6 @@ namespace AZ
path = "";
}
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Pointer pointer(path.data(), path.length());
if (pointer.IsValid())
@@ -386,9 +429,10 @@ namespace AZ
value, nullptr, valueTypeID, m_serializationSettings);
if (jsonResult.GetProcessing() != JsonSerializationResult::Processing::Halted)
{
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Value& setting = pointer.Create(m_settings, m_settings.GetAllocator());
setting = AZStd::move(store);
m_notifiers.Signal(path, Type::Object);
SignalNotifier(path, Type::Object);
return true;
}
}
@@ -404,13 +448,13 @@ namespace AZ
// Setting to empty string to prevent assert
path = "";
}
AZStd::scoped_lock lock(m_settingMutex);
rapidjson::Pointer pointerPath(path.data(), path.size());
if (!pointerPath.IsValid())
{
return false;
}
AZStd::scoped_lock lock(m_settingMutex);
return pointerPath.Erase(m_settings);
}
@@ -540,7 +584,7 @@ namespace AZ
return false;
}
m_notifiers.Signal("", Type::Object);
SignalNotifier("", Type::Object);
return true;
}
@@ -562,8 +606,6 @@ namespace AZ
scratchBuffer = &buffer;
}
AZStd::scoped_lock lock(m_settingMutex);
bool result = false;
if (path[path.length()] == 0)
{
@@ -577,6 +619,8 @@ namespace AZ
R"(Path "%.*s" is too long. Either make sure that the provided path is terminated or use a shorter path.)",
static_cast<int>(path.length()), path.data());
Pointer pointer(AZ_SETTINGS_REGISTRY_HISTORY_KEY "/-");
AZStd::scoped_lock lock(m_settingMutex);
Value pathValue(path.data(), aznumeric_caster(path.length()), m_settings.GetAllocator());
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Unable to read registry file."), m_settings.GetAllocator())
@@ -622,6 +666,7 @@ namespace AZ
{
AZ_Error("Settings Registry", false, "Folder path for the Setting Registry is too long: %.*s",
static_cast<int>(path.size()), path.data());
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Folder path for the Setting Registry is too long."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(path.data(), aznumeric_caster(path.length()), m_settings.GetAllocator()), m_settings.GetAllocator());
@@ -659,6 +704,7 @@ namespace AZ
if (fileList.size() >= MaxRegistryFolderEntries)
{
AZ_Error("Settings Registry", false, "Too many files in registry folder.");
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Too many files in registry folder."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(folderPath.c_str(), aznumeric_caster(folderPath.size()), m_settings.GetAllocator()), m_settings.GetAllocator())
@@ -678,7 +724,6 @@ namespace AZ
SystemFile::FindFiles(folderPath.c_str(), callback);
AZStd::scoped_lock lock(m_settingMutex);
if (!platform.empty())
{
// Move the folderPath prefix back to the supplied path before the wildcard
@@ -696,6 +741,7 @@ namespace AZ
if (fileList.size() >= MaxRegistryFolderEntries)
{
AZ_Error("Settings Registry", false, "Too many files in registry folder.");
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Too many files in registry folder."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(folderPath.c_str(), aznumeric_caster(folderPath.size()), m_settings.GetAllocator()), m_settings.GetAllocator())
@@ -923,6 +969,8 @@ namespace AZ
collisionFound = true;
AZ_Error("Settings Registry", false, R"(Two registry files in "%.*s" point to the same specialization: "%s" and "%s")",
AZ_STRING_ARG(folderPath), lhs.m_relativePath.c_str(), rhs.m_relativePath.c_str());
AZStd::scoped_lock lock(m_settingMutex);
historyPointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Too many files in registry folder."), m_settings.GetAllocator())
.AddMember(StringRef("Path"),
@@ -1077,6 +1125,7 @@ namespace AZ
}
}
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Unable to parse registry file due to invalid json."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator())
@@ -1102,6 +1151,7 @@ namespace AZ
R"(To merge the supplied settings registry file, the settings within it must be placed within a JSON Object '{}')"
R"( in order to allow moving of its fields using the root-key as an anchor.)", path);
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Cannot merge registry file with a root which is not a JSON Object,"
" an empty root key and a merge approach of JsonMergePatch. Otherwise the Settings Registry would be overridden."
@@ -1118,6 +1168,7 @@ namespace AZ
JsonSerializationResult::ResultCode mergeResult(JsonSerializationResult::Tasks::Merge);
if (rootKey.empty())
{
AZStd::scoped_lock lock(m_settingMutex);
mergeResult = JsonSerialization::ApplyPatch(m_settings, m_settings.GetAllocator(), jsonPatch, mergeApproach, m_applyPatchSettings);
}
else
@@ -1125,6 +1176,7 @@ namespace AZ
Pointer root(rootKey.data(), rootKey.length());
if (root.IsValid())
{
AZStd::scoped_lock lock(m_settingMutex);
Value& rootValue = root.Create(m_settings, m_settings.GetAllocator());
mergeResult = JsonSerialization::ApplyPatch(rootValue, m_settings.GetAllocator(), jsonPatch, mergeApproach, m_applyPatchSettings);
}
@@ -1132,6 +1184,7 @@ namespace AZ
{
AZ_Error("Settings Registry", false, R"(Failed to root path "%.*s" is invalid.)",
aznumeric_cast<int>(rootKey.length()), rootKey.data());
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Invalid root key."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator());
@@ -1141,15 +1194,19 @@ namespace AZ
if (mergeResult.GetProcessing() != JsonSerializationResult::Processing::Completed)
{
AZ_Error("Settings Registry", false, R"(Failed to fully merge registry file "%s".)", path);
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
.AddMember(StringRef("Error"), StringRef("Failed to fully merge registry file."), m_settings.GetAllocator())
.AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator());
return false;
}
pointer.Create(m_settings, m_settings.GetAllocator()).SetString(path, m_settings.GetAllocator());
{
AZStd::scoped_lock lock(m_settingMutex);
pointer.Create(m_settings, m_settings.GetAllocator()).SetString(path, m_settings.GetAllocator());
}
m_notifiers.Signal("", Type::Object);
SignalNotifier("", Type::Object);
return true;
}
Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h
+4 -1
View File
@@ -89,7 +89,7 @@ namespace AZ
using RegistryFileList = AZStd::fixed_vector<RegistryFile, MaxRegistryFolderEntries>;
template<typename T>
bool SetValueInternal(AZStd::string_view path, T value, SettingsRegistryInterface::Type type);
bool SetValueInternal(AZStd::string_view path, T value);
template<typename T>
bool GetValueInternal(T& result, AZStd::string_view path) const;
VisitResponse Visit(Visitor& visitor, StackedString& path, AZStd::string_view valueName,
@@ -100,8 +100,11 @@ namespace AZ
const rapidjson::Pointer& historyPointer, AZStd::string_view folderPath);
bool ExtractFileDescription(RegistryFile& output, const char* filename, const Specializations& specializations);
bool MergeSettingsFileInternal(const char* path, Format format, AZStd::string_view rootKey, AZStd::vector<char>& scratchBuffer);
void SignalNotifier(AZStd::string_view jsonPath, Type type);
mutable AZStd::recursive_mutex m_settingMutex;
mutable AZStd::recursive_mutex m_notifierMutex;
NotifyEvent m_notifiers;
rapidjson::Document m_settings;
JsonSerializerSettings m_serializationSettings;
Code/Framework/AzCore/Tests/EventTests.cpp
+31
View File
@@ -240,6 +240,37 @@ namespace UnitTest
static_assert(!AZStd::is_copy_assignable_v<AZ::Event<int32_t>>, "AZ Events should not be copy assignable");
}
TEST_F(EventTests, TestClaimHandlers_TakesAllSourceHandlers)
{
AZ::Event<> testEvent1;
AZ::Event<> testEvent2;
int32_t handlerInvokeCount{};
auto handlerCallback = [&handlerInvokeCount]()
{
++handlerInvokeCount;
};
AZ::Event<>::Handler testHandler1(handlerCallback);
AZ::Event<>::Handler testHandler2(handlerCallback);
testHandler1.Connect(testEvent1);
testHandler2.Connect(testEvent2);
EXPECT_TRUE(testEvent1.HasHandlerConnected());
EXPECT_TRUE(testEvent2.HasHandlerConnected());
testEvent1.ClaimHandlers(AZStd::move(testEvent2));
EXPECT_TRUE(testEvent1.HasHandlerConnected());
EXPECT_FALSE(testEvent2.HasHandlerConnected());
// testEvent1 should have both handlers
testEvent1.Signal();
EXPECT_EQ(2, handlerInvokeCount);
// testEvent2 should have neither of the handlers
testEvent2.Signal();
EXPECT_EQ(2, handlerInvokeCount);
}
TEST_F(EventTests, HandlerMoveAssignment_ProperlyDisconnectsFromOldEvent)
{
AZ::Event<> testEvent1;