From 906042359243d215cd05303f732a3422a8fa20a1 Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Fri, 13 Aug 2021 16:20:08 -0500 Subject: [PATCH] Settings registry notification deadlock fix (#3065) * 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> --- Code/Framework/AzCore/AzCore/EBus/Event.h | 6 + Code/Framework/AzCore/AzCore/EBus/Event.inl | 26 ++++ .../AzCore/Settings/SettingsRegistryImpl.cpp | 111 +++++++++++++----- .../AzCore/Settings/SettingsRegistryImpl.h | 5 +- Code/Framework/AzCore/Tests/EventTests.cpp | 31 +++++ 5 files changed, 151 insertions(+), 28 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/EBus/Event.h b/Code/Framework/AzCore/AzCore/EBus/Event.h index b3c29b63a2..00310f4dbc 100644 --- a/Code/Framework/AzCore/AzCore/EBus/Event.h +++ b/Code/Framework/AzCore/AzCore/EBus/Event.h @@ -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; diff --git a/Code/Framework/AzCore/AzCore/EBus/Event.inl b/Code/Framework/AzCore/AzCore/EBus/Event.inl index dfb1781991..ffa8c8b9c5 100644 --- a/Code/Framework/AzCore/AzCore/EBus/Event.inl +++ b/Code/Framework/AzCore/AzCore/EBus/Event.inl @@ -207,6 +207,32 @@ namespace AZ } + template + auto Event::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* handlerList : handlerContainers) + { + for (Handler* handler : *handlerList) + { + if (handler != nullptr) + { + handler->m_index = 0; + handler->m_event = this; + Connect(*handler); + } + } + } + + return *this; + } + + template bool Event::HasHandlerConnected() const { diff --git a/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp b/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp index 0882e1b7f2..3dc2d4145e 100644 --- a/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp +++ b/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp @@ -20,7 +20,7 @@ namespace AZ { template - 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, "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(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(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(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; } diff --git a/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h b/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h index 41a628cf22..4126e19322 100644 --- a/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h +++ b/Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h @@ -89,7 +89,7 @@ namespace AZ using RegistryFileList = AZStd::fixed_vector; template - bool SetValueInternal(AZStd::string_view path, T value, SettingsRegistryInterface::Type type); + bool SetValueInternal(AZStd::string_view path, T value); template 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& 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; diff --git a/Code/Framework/AzCore/Tests/EventTests.cpp b/Code/Framework/AzCore/Tests/EventTests.cpp index 358c1a62ae..aaa70344e7 100644 --- a/Code/Framework/AzCore/Tests/EventTests.cpp +++ b/Code/Framework/AzCore/Tests/EventTests.cpp @@ -240,6 +240,37 @@ namespace UnitTest static_assert(!AZStd::is_copy_assignable_v>, "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;