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) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index ba17ceb124..fd79b209a7 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -578,44 +578,67 @@ private: std::vector newLoadedObjects; std::vector badObjects; - // Read objects - std::mutex commonMutex; - ParallelFor(requiredObjects, [&](size_t i) { - auto& otl = requiredObjects[i]; - auto* requiredObject = otl.RepositoryItem; - if (requiredObject != nullptr) + // Create a list of objects that are currently not loaded but required. + std::vector objectsToLoad; + for (auto& requiredObject : requiredObjects) + { + auto* repositoryItem = requiredObject.RepositoryItem; + if (repositoryItem == 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 = repositoryItem->LoadedObject.get(); + if (loadedObject == nullptr) + { + objectsToLoad.push_back(repositoryItem); + } + } + + // 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; + objects.push_back(loadedObject); + } + // Load objects for (auto* obj : newLoadedObjects) {