Merge pull request #256 from aws-lumberyard-dev/amzn-mike/lyn-2249-disable-cancel

Disable Asset Load Cancellation
2021-04-23 13:10:56 -05:00
parent 0629e20b35 d25179303d
commit 16ca036ea8
8 changed files with 162 additions and 18 deletions
@@ -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);
                }
@@ -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<AssetData> asset)
        {
+            AZ_Warning("AssetContainer", false, "Error loading asset %s", asset->GetId().ToString<AZStd::string>().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<AZStd::string>().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());
+                    }
                }
            }
        }
@@ -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<AZStd::mutex> lock(m_readRequestMutex);
+        if (m_curReadRequest)
+        {
+            auto streamer = Interface<IO::IStreamer>::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);
@@ -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);
@@ -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;
@@ -2144,7 +2144,7 @@ namespace AZ
            if (curIter != m_assetContainers.end())
            {
                auto newRef = curIter->second.lock();
-                if (newRef)
+                if (newRef && newRef->IsValid())
                {
                    return newRef;
                }
@@ -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);
@@ -575,6 +575,91 @@ namespace UnitTest
        EXPECT_EQ(baseStatus, expected_base_status);
    }

+    struct DebugListener : AZ::Interface<IDebugAssetEvent>::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<AZStd::string>().c_str(), static_cast<int>(status)).c_str());
+        }
+        void ReleaseAsset(AZ::Data::AssetId id) override
+        {
+            AZ::Debug::Trace::Output(
+                "", AZStd::string::format("Release %s\n", id.ToString<AZStd::string>().c_str()).c_str());
+        }
+    };
+
+    TEST_F(AssetJobsFloodTest, RapidAcquireAndRelease)
+    {
+        DebugListener listener;  
+        auto assetUuids = {
+            MyAsset1Id,
+            MyAsset2Id,
+            MyAsset3Id,
+        };
+
+        AZStd::vector<AZStd::thread> threads;
+        AZStd::mutex mutex;
+        AZStd::atomic<int> threadCount(static_cast<int>(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<AssetWithAssetReference> asset1 =
+                        m_testAssetManager->GetAsset(assetUuid, azrtti_typeid<AssetWithAssetReference>(), AZ::Data::AssetLoadBehavior::PreLoad);
+                    
+                    if (checkLoaded)
+                    {
+                        asset1.BlockUntilLoadComplete();
+
+                        EXPECT_TRUE(asset1.IsReady()) << "Iteration " << i << " failed.  Asset status: " <<  static_cast<int>(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<AZStd::mutex> 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<AssetWithAssetReference>());