From 1dd9fd525f0df4ea317739308866c65667d4ad7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Tue, 28 Mar 2023 23:18:16 +0300 Subject: [PATCH 1/4] Fix #19296: Race condition for parallel object loading --- src/openrct2/object/ObjectManager.cpp | 88 +++++++++++++++++---------- 1 file changed, 55 insertions(+), 33 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index ba17ceb124..6f7bd55489 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -578,44 +578,66 @@ private: std::vector newLoadedObjects; std::vector badObjects; - // Read objects - std::mutex commonMutex; - ParallelFor(requiredObjects, [&](size_t i) { - auto& otl = requiredObjects[i]; + // Create a list of objects that are currently not loaded but required. + std::vector objectsToLoad; + for (auto& otl : requiredObjects) + { auto* requiredObject = otl.RepositoryItem; - if (requiredObject != nullptr) + if (requiredObject == nullptr) { - auto* loadedObject = requiredObject->LoadedObject.get(); - if (loadedObject == nullptr) - { - // Object requires to be loaded, if the object successfully loads it will register it - // as a loaded object otherwise placed into the badObjects list. - auto newObject = _objectRepository.LoadObject(requiredObject); - std::lock_guard guard(commonMutex); - if (newObject == nullptr) - { - badObjects.push_back(ObjectEntryDescriptor(requiredObject->ObjectEntry)); - ReportObjectLoadProblem(&requiredObject->ObjectEntry); - } - else - { - otl.LoadedObject = newObject.get(); - newLoadedObjects.push_back(otl.LoadedObject); - // Connect the ori to the registered object - _objectRepository.RegisterLoadedObject(requiredObject, std::move(newObject)); - } - } - else - { - otl.LoadedObject = loadedObject; - } - { - std::lock_guard guard(commonMutex); - objects.push_back(loadedObject); - } + continue; + } + + auto* loadedObject = requiredObject->LoadedObject.get(); + if (loadedObject == nullptr) + { + objectsToLoad.push_back(requiredObject); + } + } + + // De-duplicate the list, since loading happens in parallel we can't have it race the repository item. + std::sort(objectsToLoad.begin(), objectsToLoad.end()); + objectsToLoad.erase(std::unique(objectsToLoad.begin(), objectsToLoad.end()), objectsToLoad.end()); + + // Load the objects. + std::mutex commonMutex; + ParallelFor(objectsToLoad, [&](size_t i) { + const auto* requiredObject = objectsToLoad[i]; + + // Object requires to be loaded, if the object successfully loads it will register it + // as a loaded object otherwise placed into the badObjects list. + auto newObject = _objectRepository.LoadObject(requiredObject); + + std::lock_guard guard(commonMutex); + if (newObject == nullptr) + { + badObjects.push_back(ObjectEntryDescriptor(requiredObject->ObjectEntry)); + ReportObjectLoadProblem(&requiredObject->ObjectEntry); + } + else + { + newLoadedObjects.push_back(newObject.get()); + // Connect the ori to the registered object + _objectRepository.RegisterLoadedObject(requiredObject, std::move(newObject)); } }); + // Assign the loaded objects to the required objects + for (auto& requiredObject : requiredObjects) + { + auto* repositoryItem = requiredObject.RepositoryItem; + if (repositoryItem == nullptr) + { + continue; + } + auto* loadedObject = repositoryItem->LoadedObject.get(); + if (loadedObject == nullptr) + { + continue; + } + requiredObject.LoadedObject = loadedObject; + } + // Load objects for (auto* obj : newLoadedObjects) { From e69d46bd41dd4394bdd3c4e2e96362fa3bf7b6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 29 Mar 2023 00:35:10 +0300 Subject: [PATCH 2/4] Rename some variables --- src/openrct2/object/ObjectManager.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index 6f7bd55489..760c4e5255 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -580,18 +580,18 @@ private: // Create a list of objects that are currently not loaded but required. std::vector objectsToLoad; - for (auto& otl : requiredObjects) + for (auto& requiredObject : requiredObjects) { - auto* requiredObject = otl.RepositoryItem; - if (requiredObject == nullptr) + auto* repositoryItem = requiredObject.RepositoryItem; + if (repositoryItem == nullptr) { continue; } - auto* loadedObject = requiredObject->LoadedObject.get(); + auto* loadedObject = repositoryItem->LoadedObject.get(); if (loadedObject == nullptr) { - objectsToLoad.push_back(requiredObject); + objectsToLoad.push_back(repositoryItem); } } From 92f821af0729c34b099517da8362776bc0fdef42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 29 Mar 2023 02:33:40 +0300 Subject: [PATCH 3/4] Fix some mistake --- src/openrct2/object/ObjectManager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index 760c4e5255..fd79b209a7 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -636,6 +636,7 @@ private: continue; } requiredObject.LoadedObject = loadedObject; + objects.push_back(loadedObject); } // Load objects From 0669e473241b43921f67454b44dfbbbe0ba59092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 29 Mar 2023 15:56:02 +0300 Subject: [PATCH 4/4] Update changelog.txt --- distribution/changelog.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 43b34ad599..0c1a323546 100644 --- a/distribution/changelog.txt +++ b/distribution/changelog.txt @@ -1,5 +1,6 @@ 0.4.5 (in development) ------------------------------------------------------------------------ +- Fix: [#19296] Crash due to a race condition for parallel object loading. - Fix: [#19756] Crash with title sequences containing no commands. 0.4.4 (2023-03-28)