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

[LYN-2878] Attempt to fix deadlocks that occur when the Editor loads surface tag assets.

Browse Source 
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<T> 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.
This commit is contained in:
mbalfour
2021-04-13 13:03:43 -05:00
parent a1c664e666
commit d7f10e9d60
3 changed files with 21 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Gems/SurfaceData/Code/Source/Editor/EditorSurfaceDataSystemComponent.cpp
+19 -7
View File
@@ -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<AZ::Data::AssetId> 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<EditorSurfaceTagListAsset>();
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<EditorSurfaceTagListAsset>(), 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);
}
}
}