diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h index c09d531e0a..c8a245af68 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h @@ -1104,8 +1104,11 @@ namespace AZ // If we either already had valid asset data, or just created it via FindOrCreateAsset, try to queue the load. if (m_assetData && m_assetData->GetId().IsValid()) { - // Only try to queue if the asset isn't already loading or loaded. - if (m_assetData->GetStatus() == AZ::Data::AssetData::AssetStatus::NotLoaded) + // Try to queue if the asset isn't already loading or loaded. + // Also try to queue if the asset *is* already loading or loaded, but we're the only one with a strong reference + // (i.e. use count == 1), because that means it was in the process of being garbage-collected. + if ((m_assetData->GetStatus() == AZ::Data::AssetData::AssetStatus::NotLoaded) || + (m_assetData->GetUseCount() == 1)) { *this = AssetInternal::GetAsset(m_assetData->GetId(), m_assetData->GetType(), loadBehavior, loadParams); } diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp index 984aeeb756..4e7a19c197 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp @@ -255,7 +255,7 @@ namespace AZ bool AssetContainer::IsValid() const { - return (m_containerAssetId.IsValid() && m_initComplete); + return (m_containerAssetId.IsValid() && m_initComplete && m_rootAsset); } void AssetContainer::CheckReady() @@ -341,6 +341,7 @@ namespace AZ void AssetContainer::OnAssetError(Asset asset) { + AZ_Warning("AssetContainer", false, "Error loading asset %s", asset->GetId().ToString().c_str()); HandleReadyAsset(asset); } @@ -366,7 +367,10 @@ namespace AZ auto remainingPreloadIter = m_preloadList.find(waiterId); if (remainingPreloadIter == m_preloadList.end()) { - AZ_Warning("AssetContainer", !m_initComplete, "Couldn't find waiting list for %s", waiterId.ToString().c_str()); + // If we got here without an entry on the preload list, it probably means this asset was triggered to load multiple + // times, some with dependencies and some without. To ensure that we don't disturb the loads that expect the + // dependencies, just silently return and don't treat the asset as finished loading. We'll rely on the other load + // to send an OnAssetReady() whenever its expected dependencies are met. return; } if (!remainingPreloadIter->second.erase(preloadID)) @@ -610,7 +614,12 @@ namespace AZ } for(auto& thisList : preloadList) { - m_preloadList[thisList.first].insert(thisList.second.begin(), thisList.second.end()); + // Only save the entry to the final preload list if it has at least one dependent asset still remaining after + // the checks above. + if (!thisList.second.empty()) + { + m_preloadList[thisList.first].insert(thisList.second.begin(), thisList.second.end()); + } } } } diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp index 5ef278c7ea..a17ff6fd6e 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp @@ -208,6 +208,7 @@ namespace AZ::Data void AssetDataStream::Close() { AZ_Assert(m_isOpen, "Attempting to close a stream that hasn't been opened."); + AZ_Assert(m_curReadRequest == nullptr, "Attempting to close a stream with a read request in flight."); // Destroy the asset buffer and unlock the allocator, so the allocator itself knows that it is no longer needed. if (m_buffer != m_preloadedData.data()) @@ -222,6 +223,16 @@ namespace AZ::Data AZ_PROFILE_INTERVAL_END(AZ::Debug::ProfileCategory::AzCore, this); } + void AssetDataStream::RequestCancel() + { + AZStd::scoped_lock lock(m_readRequestMutex); + if (m_curReadRequest) + { + auto streamer = Interface::Get(); + m_curReadRequest = streamer->Cancel(m_curReadRequest); + } + } + void AssetDataStream::Seek(AZ::IO::OffsetType bytes, AZ::IO::GenericStream::SeekMode mode) { AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzCore); diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h index 58144c2f92..1367c493f5 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h @@ -82,6 +82,10 @@ namespace AZ::Data //! Gets the size of data loaded (so far). size_t GetLoadedSize() const { return m_loadedSize; } + //! Request a cancellation of any current IO streamer requests. + //! Note: This is asynchronous and not guaranteed to cancel if the request is already in-process. + void RequestCancel(); + private: //! Perform any operations needed by all variants of Open() void OpenInternal(size_t assetSize, const char* streamName); diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h index 1384511e78..b772d5a8cb 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h @@ -28,6 +28,8 @@ namespace AZ::Data::AssetInternal class WeakAsset { public: + static constexpr bool EnableAssetCancellation = false; + WeakAsset() = default; WeakAsset(AssetData* assetData, AssetLoadBehavior assetReferenceLoadBehavior); @@ -111,7 +113,14 @@ namespace AZ::Data::AssetInternal // - If the left and right sides are the same, clearing the right side's reference means one less reference will exist if (m_assetData) { - m_assetData->ReleaseWeak(); + if constexpr (EnableAssetCancellation) + { + m_assetData->ReleaseWeak(); + } + else + { + m_assetData->Release(); + } } m_assetData = AZStd::move(rhs.m_assetData); rhs.m_assetData = nullptr; @@ -141,13 +150,27 @@ namespace AZ::Data::AssetInternal if (assetData) { - assetData->AcquireWeak(); + if constexpr (EnableAssetCancellation) + { + assetData->AcquireWeak(); + } + else + { + assetData->Acquire(); + } m_assetId = assetData->GetId(); } if (m_assetData) { - m_assetData->ReleaseWeak(); + if constexpr (EnableAssetCancellation) + { + m_assetData->ReleaseWeak(); + } + else + { + m_assetData->Release(); + } } m_assetData = assetData; diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp index 214443142b..db8736b4a8 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp @@ -2144,7 +2144,7 @@ namespace AZ if (curIter != m_assetContainers.end()) { auto newRef = curIter->second.lock(); - if (newRef) + if (newRef && newRef->IsValid()) { return newRef; } diff --git a/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp b/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp index d0e34487ea..c829821ca6 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp @@ -186,7 +186,8 @@ namespace UnitTest int GetWeakUseCount() { return m_weakUseCount.load(); } }; - TEST_F(WeakAssetTest, WeakAsset_ConstructionAndDestruction_UpdatesAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_ConstructionAndDestruction_UpdatesAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -202,7 +203,8 @@ namespace UnitTest EXPECT_EQ(testData.GetWeakUseCount(), 0); } - TEST_F(WeakAssetTest, WeakAsset_MoveOperatorWithDifferentData_UpdatesOldAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_MoveOperatorWithDifferentData_UpdatesOldAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -217,7 +219,8 @@ namespace UnitTest AZ_TEST_STOP_TRACE_SUPPRESSION(1); } - TEST_F(WeakAssetTest, WeakAsset_MoveOperatorWithSameData_PreservesAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_MoveOperatorWithSameData_PreservesAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -234,7 +237,8 @@ namespace UnitTest AZ_TEST_STOP_TRACE_SUPPRESSION(1); } - TEST_F(WeakAssetTest, WeakAsset_AssignmentOperator_CopiesDataAndIncrementsWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_AssignmentOperator_CopiesDataAndIncrementsWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index 18e193304a..78e2288a4c 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -575,6 +575,91 @@ namespace UnitTest EXPECT_EQ(baseStatus, expected_base_status); } + struct DebugListener : AZ::Interface::Registrar + { + void AssetStatusUpdate(AZ::Data::AssetId id, AZ::Data::AssetData::AssetStatus status) override + { + AZ::Debug::Trace::Output( + "", AZStd::string::format("Status %s - %d\n", id.ToString().c_str(), static_cast(status)).c_str()); + } + void ReleaseAsset(AZ::Data::AssetId id) override + { + AZ::Debug::Trace::Output( + "", AZStd::string::format("Release %s\n", id.ToString().c_str()).c_str()); + } + }; + + TEST_F(AssetJobsFloodTest, RapidAcquireAndRelease) + { + DebugListener listener; + auto assetUuids = { + MyAsset1Id, + MyAsset2Id, + MyAsset3Id, + }; + + AZStd::vector threads; + AZStd::mutex mutex; + AZStd::atomic threadCount(static_cast(assetUuids.size())); + AZStd::condition_variable cv; + AZStd::atomic_bool keepDispatching(true); + + auto dispatch = [&keepDispatching]() { + while (keepDispatching) + { + AssetManager::Instance().DispatchEvents(); + } + }; + + AZStd::thread dispatchThread(dispatch); + + for (const auto& assetUuid : assetUuids) + { + threads.emplace_back([this, &threadCount, &cv, assetUuid]() { + bool checkLoaded = true; + + for (int i = 0; i < 5000; i++) + { + Asset asset1 = + m_testAssetManager->GetAsset(assetUuid, azrtti_typeid(), AZ::Data::AssetLoadBehavior::PreLoad); + + if (checkLoaded) + { + asset1.BlockUntilLoadComplete(); + + EXPECT_TRUE(asset1.IsReady()) << "Iteration " << i << " failed. Asset status: " << static_cast(asset1.GetStatus()); + } + + checkLoaded = !checkLoaded; + } + + --threadCount; + cv.notify_one(); + }); + } + + bool timedOut = false; + + // Used to detect a deadlock. If we wait for more than 5 seconds, it's likely a deadlock has occurred + while (threadCount > 0 && !timedOut) + { + AZStd::unique_lock lock(mutex); + timedOut = (AZStd::cv_status::timeout == cv.wait_until(lock, AZStd::chrono::system_clock::now() + DefaultTimeoutSeconds * 20000)); + } + + ASSERT_EQ(threadCount, 0) << "Thread count is non-zero, a thread has likely deadlocked. Test will not shut down cleanly."; + + for (auto& thread : threads) + { + thread.join(); + } + + keepDispatching = false; + dispatchThread.join(); + + AssetManager::Destroy(); + } + #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetJobsFloodTest, DISABLED_AssetLoadBehaviorIsPreserved) #else @@ -2592,7 +2677,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #else - TEST_F(AssetManagerCancelTests, CancelLoad_NoReferences_LoadCancels) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 + TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 100); @@ -2632,7 +2718,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #else - TEST_F(AssetManagerCancelTests, CanceledLoad_CanBeLoadedAgainLater) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 + TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 50); @@ -2681,7 +2768,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #else - TEST_F(AssetManagerCancelTests, CancelLoad_InProgressLoad_Continues) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 + TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 100); @@ -2903,8 +2991,9 @@ namespace UnitTest TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #else + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerClearAssetReferenceTests, - ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) + DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { // Start the load and wait for the dependent asset to hit the loading state. @@ -2961,7 +3050,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #else - TEST_F(AssetManagerClearAssetReferenceTests, ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 + TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { OnAssetReadyListener assetStatus1(DependentPreloadAssetId, azrtti_typeid());