diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp index 4e7a19c197..50c382d7da 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp @@ -239,8 +239,13 @@ namespace AZ return; } - CheckReady(); m_initComplete = true; + + // *After* setting initComplete to true, check to see if the assets are already ready. + // This check needs to wait until after setting initComplete because if they *are* ready, we want the final call to + // RemoveWaitingAsset to trigger the OnAssetContainer* event. If we call CheckReady() *before* setting initComplete, + // if all the assets are ready, the event will never get triggered. + CheckReady(); } bool AssetContainer::IsReady() const diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerStreamingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerStreamingTests.cpp index 807288ff82..f5e2a880d5 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerStreamingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerStreamingTests.cpp @@ -330,4 +330,163 @@ namespace UnitTest } } -} + // The AssetManagerStreamerImmediateCompletionTests class adjusts the asset loading to force it to complete immediately, + // while still within the callstack for GetAsset(). This can be used to test various conditions in which the load thread + // completes more rapidly than expected, and can expose subtle race conditions. + // There are a few key things that this class does to make this work: + // - The file I/O streamer is mocked + // - The asset stream data is mocked to a 0-byte length for the asset so that the stream load will bypass the I/O streamer and + // just immediately return completion. + // - The number of JobManager threads is set to 0, forcing jobs to execute synchronously inline when they are started. + // With these changes, GetAssetInternal() will queue the stream, which will immediately call the callback that creates LoadAssetJob, + // which immediately executes in-place to process the asset due to the synchronous JobManager. + // Note that if we just created the asset in a Ready state, most of the asset loading code is completely bypassed, and so we + // wouldn't be able to test for race conditions in the AssetContainer. + // + // This class also unregisters the catalog and asset handler before shutting down the asset manager. This is done to catch + // any outstanding asset references that exist due to loads not completing and cleaning up successfully. + struct AssetManagerStreamerImmediateCompletionTests : public BaseAssetManagerTest, + public AZ::Data::AssetCatalogRequestBus::Handler, + public AZ::Data::AssetHandler, + public AZ::Data::AssetCatalog + { + static inline const AZ::Uuid TestAssetId{"{E970B177-5F45-44EB-A2C4-9F29D9A0B2A2}"}; + static inline const char* TestAssetPath{"test"}; + + void SetUp() override + { + BaseAssetManagerTest::SetUp(); + AssetManager::Descriptor desc; + AssetManager::Create(desc); + + // Register the handler and catalog after creation, because we intend to destroy them before AssetManager destruction. + // The specific asset we load is irrelevant, so register EmptyAsset. + AZ::Data::AssetManager::Instance().RegisterHandler(this, AZ::AzTypeInfo::Uuid()); + AZ::Data::AssetManager::Instance().RegisterCatalog(this, AZ::AzTypeInfo::Uuid()); + + // Intercept messages for finding assets by name so that we can mock out the asset we're loading. + AZ::Data::AssetCatalogRequestBus::Handler::BusConnect(); + } + + void TearDown() override + { + // Unregister before destroying AssetManager. + // This will catch any assets that got stuck in a loading state without getting cleaned up. + AZ::Data::AssetManager::Instance().UnregisterCatalog(this); + AZ::Data::AssetManager::Instance().UnregisterHandler(this); + + AZ::Data::AssetCatalogRequestBus::Handler::BusDisconnect(); + + AssetManager::Destroy(); + BaseAssetManagerTest::TearDown(); + } + + size_t GetNumJobManagerThreads() const override + { + // Return 0 threads so that the Job Manager executes jobs synchronously inline. This lets us finish a load while still + // in the callstack that initiates the load. + return 0; + } + + // Create a mock streamer instead of a real one, since we don't really want to load an asset. + IO::IStreamer* CreateStreamer() override + { + m_mockStreamer = AZStd::make_unique(); + return &(m_mockStreamer->m_mockStreamer); + } + + void DestroyStreamer([[maybe_unused]] IO::IStreamer* streamer) override + { + m_mockStreamer = nullptr; + } + + // AssetHandler implementation + + // Minimalist mock to create a new EmptyAsset with the desired asset ID. + AZ::Data::AssetPtr CreateAsset(const AZ::Data::AssetId& id, [[maybe_unused]] const AZ::Data::AssetType& type) override + { + return new EmptyAsset(id); + } + + void DestroyAsset(AZ::Data::AssetPtr ptr) override + { + delete ptr; + } + + // The mocked-out Asset Catalog handles EmptyAsset types. + void GetHandledAssetTypes(AZStd::vector& assetTypes) override + { + assetTypes.push_back(AZ::AzTypeInfo::Uuid()); + } + + // This is a mocked-out load, so just immediately return completion without doing anything. + AZ::Data::AssetHandler::LoadResult LoadAssetData( + [[maybe_unused]] const AZ::Data::Asset& asset, + [[maybe_unused]] AZStd::shared_ptr stream, + [[maybe_unused]] const AZ::Data::AssetFilterCB& assetLoadFilterCB) + { + return AZ::Data::AssetHandler::LoadResult::LoadComplete; + } + + // AssetCatalogRequestBus implementation + + // Minimalist mocks to provide our desired asset path or asset id + AZStd::string GetAssetPathById([[maybe_unused]] const AZ::Data::AssetId& id) override + { + return TestAssetPath; + } + AZ::Data::AssetId GetAssetIdByPath( + [[maybe_unused]] const char* path, [[maybe_unused]] const AZ::Data::AssetType& typeToRegister, + [[maybe_unused]] bool autoRegisterIfNotFound) override + { + return TestAssetId; + } + + // Return the mocked-out information for our test asset + AZ::Data::AssetInfo GetAssetInfoById([[maybe_unused]] const AZ::Data::AssetId& id) override + { + AZ::Data::AssetInfo assetInfo; + assetInfo.m_assetId = TestAssetId; + assetInfo.m_assetType = AZ::AzTypeInfo::Uuid(); + assetInfo.m_relativePath = TestAssetPath; + return assetInfo; + } + + // AssetCatalog implementation + + // Set the mocked-out asset load to have a 0-byte length so that the load skips I/O and immediately returns success + AZ::Data::AssetStreamInfo GetStreamInfoForLoad( + [[maybe_unused]] const AZ::Data::AssetId& id, const AZ::Data::AssetType& type) override + { + EXPECT_TRUE(type == AZ::AzTypeInfo::Uuid()); + AZ::Data::AssetStreamInfo info; + info.m_dataOffset = 0; + info.m_streamName = TestAssetPath; + info.m_dataLen = 0; + info.m_streamFlags = AZ::IO::OpenMode::ModeRead; + + return info; + } + + AZStd::unique_ptr m_mockStreamer; + }; + + // This test will verify that even if the asset loading stream/job returns immediately, all of the loading + // code works successfully. The test here is fairly simple - it just loads the asset and verifies that it + // loaded successfully. The bulk of the test is really in the setup class above, where the load is forced + // to complete immediately. Also, the true failure condition is caught in the setup class too, which is + // the presence of any assets at the point that the asset handler is unregistered. If they're present, then + // the immediate load wasn't truly successful, as it left around extra references to the asset that haven't + // been cleaned up. + TEST_F(AssetManagerStreamerImmediateCompletionTests, LoadAssetWithImmediateJobCompletion_WorksSuccessfully) + { + AZ::Data::AssetLoadParameters loadParams; + + auto testAsset = + AssetManager::Instance().GetAsset(TestAssetId, AZ::Data::AssetLoadBehavior::Default, loadParams); + + AZ::Data::AssetManager::Instance().DispatchEvents(); + EXPECT_TRUE(testAsset.IsReady()); + } + +} // namespace UnitTest diff --git a/Code/Framework/AzCore/Tests/Asset/TestAssetTypes.h b/Code/Framework/AzCore/Tests/Asset/TestAssetTypes.h index 83f3909676..d4267972ec 100644 --- a/Code/Framework/AzCore/Tests/Asset/TestAssetTypes.h +++ b/Code/Framework/AzCore/Tests/Asset/TestAssetTypes.h @@ -24,6 +24,13 @@ namespace UnitTest public: AZ_CLASS_ALLOCATOR(EmptyAsset, AZ::SystemAllocator, 0); AZ_RTTI(EmptyAsset, "{098E3F7F-13AC-414B-9B4E-49B5AD1BD7FE}", AZ::Data::AssetData); + + EmptyAsset( + const AZ::Data::AssetId& assetId = AZ::Data::AssetId(), + AZ::Data::AssetData::AssetStatus status = AZ::Data::AssetData::AssetStatus::NotLoaded) + : AZ::Data::AssetData(assetId, status) + { + } }; // EmptyAssetWithNoHandler: no data contained within, and no AssetHandler registered for this type diff --git a/Gems/Vegetation/Code/Tests/DynamicSliceInstanceSpawnerTests.cpp b/Gems/Vegetation/Code/Tests/DynamicSliceInstanceSpawnerTests.cpp index ef2b52f05d..8c922359cf 100644 --- a/Gems/Vegetation/Code/Tests/DynamicSliceInstanceSpawnerTests.cpp +++ b/Gems/Vegetation/Code/Tests/DynamicSliceInstanceSpawnerTests.cpp @@ -275,7 +275,7 @@ namespace UnitTest } // [SPEC-6600] This test intermittently fails on automated builds, so disabling temporarily until root cause identified - TEST_F(DynamicSliceInstanceSpawnerTests, DISABLED_DifferentSpawnersAreNotEqual) + TEST_F(DynamicSliceInstanceSpawnerTests, DifferentSpawnersAreNotEqual) { // Two spawners with different data should *not* be data-equivalent. @@ -291,7 +291,7 @@ namespace UnitTest // [LY-118267] This test intermittently fails on automated builds, so disabling temporarily until the root cause // can be identified - TEST_F(DynamicSliceInstanceSpawnerTests, DISABLED_LoadAndUnloadAssets) + TEST_F(DynamicSliceInstanceSpawnerTests, LoadAndUnloadAssets) { // The spawner should successfully load/unload assets without errors. @@ -312,7 +312,7 @@ namespace UnitTest } // [SPEC-6600] This test intermittently fails on automated builds, so disabling temporarily until root cause identified - TEST_F(DynamicSliceInstanceSpawnerTests, DISABLED_CreateAndDestroyInstance) + TEST_F(DynamicSliceInstanceSpawnerTests, CreateAndDestroyInstance) { // The spawner should successfully create and destroy an instance without errors.