From 0f841656e3a627c656233d58c1e231553dbf81f0 Mon Sep 17 00:00:00 2001 From: mbalfour Date: Tue, 13 Apr 2021 13:03:43 -0500 Subject: [PATCH] [LYN-2878] Attempt to fix deadlocks that occur when the Editor loads surface tag assets. I wasn't able to reproduce the deadlock, but from the reported callstack, the following lock inversion happens: * EditorSurfaceDataSystemComponent::OnCatalogLoaded locked the AssetCatalogRequestBus mutex by calling EnumerateAssets, and then locked m_assetMutex inside GetAsset->FindOrCreateAsset inside the enumerate callback. * Loading threads would lock m_assetMutex in AssetManager::ValidateAndRegisterAssetLoading, then lock the AssetCatalogRequestBus inside the Asset copy constructor when calling UpdateDebugStatus when the constructor calls SetData->UpgradeAssetInfo->UpdateAssetInfo->AssetCatalogRequestBus::GetAssetInfoById This should solve the lock inversion on both sides of the problem: * UpdateDebugStatus now takes in a const ref instead of a copy, so the copy constructor isn't called. * EditorSurfaceDataSystemComponent::OnCatalogLoaded is rewritten to call GetAsset outside of the enumeration call. As a bonus, this also removes the blocking load call. The rest of the code already supports asynchronous refreshes as the list assets are added / modified / removed, so this code was changed to leverage the asynchronous refreshes as well. --- .../AzCore/AzCore/Asset/AssetManager.cpp | 2 +- .../AzCore/AzCore/Asset/AssetManager.h | 2 +- .../EditorSurfaceDataSystemComponent.cpp | 26 ++++++++++++++----- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp index 9d11eef6fc..87c5c6cb03 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp @@ -1117,7 +1117,7 @@ namespace AZ return asset; } - void AssetManager::UpdateDebugStatus(AZ::Data::Asset asset) + void AssetManager::UpdateDebugStatus(const AZ::Data::Asset& asset) { if(!m_debugAssetEvents) { diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetManager.h b/Code/Framework/AzCore/AzCore/Asset/AssetManager.h index 7df2972a3d..81568d936e 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetManager.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetManager.h @@ -358,7 +358,7 @@ namespace AZ Asset GetAssetInternal(const AssetId& assetId, const AssetType& assetType, AssetLoadBehavior assetReferenceLoadBehavior, const AssetLoadParameters& loadParams = AssetLoadParameters{}, AssetInfo assetInfo = AssetInfo(), bool signalLoaded = false); - void UpdateDebugStatus(AZ::Data::Asset asset); + void UpdateDebugStatus(const AZ::Data::Asset& asset); /** * Gets a root asset and dependencies as individual async loads if necessary. diff --git a/Gems/SurfaceData/Code/Source/Editor/EditorSurfaceDataSystemComponent.cpp b/Gems/SurfaceData/Code/Source/Editor/EditorSurfaceDataSystemComponent.cpp index 4f62550216..4dcc969434 100644 --- a/Gems/SurfaceData/Code/Source/Editor/EditorSurfaceDataSystemComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Editor/EditorSurfaceDataSystemComponent.cpp @@ -145,24 +145,36 @@ namespace SurfaceData void EditorSurfaceDataSystemComponent::OnCatalogLoaded(const char* /*catalogFile*/) { - //automatically register all surface tag list assets + //automatically register all existing surface tag list assets at Editor startup - // First run through all the assets and trigger loads on them. + AZStd::vector surfaceTagAssetIds; + + // First run through all the assets and gather up the asset IDs for all surface tag list assets AZ::Data::AssetCatalogRequestBus::Broadcast(&AZ::Data::AssetCatalogRequestBus::Events::EnumerateAssets, nullptr, - [this](const AZ::Data::AssetId assetId, const AZ::Data::AssetInfo& assetInfo) { + [&surfaceTagAssetIds](const AZ::Data::AssetId assetId, const AZ::Data::AssetInfo& assetInfo) { const auto assetType = azrtti_typeid(); if (assetInfo.m_assetType == assetType) { - m_surfaceTagNameAssets[assetId] = AZ::Data::AssetManager::Instance().GetAsset(assetId, assetType, AZ::Data::AssetLoadBehavior::Default); + surfaceTagAssetIds.emplace_back(assetId); } }, nullptr); - // After all the loads are triggered, block to make sure they've all completed. - for (auto& asset : m_surfaceTagNameAssets) + // Next, trigger all the loads. This is done outside of EnumerateAssets to ensure that we don't have any deadlocks caused by + // lock inversion. If this thread locks AssetCatalogRequestBus mutex with EnumerateAssets, then locks m_assetMutex in + // AssetManager::FindOrCreateAsset, it's possible for those locks to get locked in reverse on a loading thread, causing a deadlock. + for (auto& assetId : surfaceTagAssetIds) { - asset.second.BlockUntilLoadComplete(); + m_surfaceTagNameAssets[assetId] = AZ::Data::AssetManager::Instance().GetAsset( + assetId, azrtti_typeid(), AZ::Data::AssetLoadBehavior::Default); + + // If any assets are still loading (which they likely will be), listen for the OnAssetReady event and refresh the Editor + // UI as each one finishes loading. + if (!m_surfaceTagNameAssets[assetId].IsReady()) + { + AZ::Data::AssetBus::MultiHandler::BusConnect(assetId); + } } }