Merge pull request #19757 from ZehMatt/fix-19296

Fix #19296: Race condition for parallel object loading
OpenRCT2 · Mar 30, 2023 · 63b0d16 · 63b0d16
2 parents 7275377 + 0669e47
commit 63b0d16
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 33 deletions.
diff --git 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
@@ -578,44 +578,67 @@ class ObjectManager final : public IObjectManager
         std::vector<Object*> newLoadedObjects;
         std::vector<ObjectEntryDescriptor> badObjects;
 
-        // Read objects
+        // Create a list of objects that are currently not loaded but required.
+        std::vector<const ObjectRepositoryItem*> objectsToLoad;
+        for (auto& requiredObject : requiredObjects)
+        {
+            auto* repositoryItem = requiredObject.RepositoryItem;
+            if (repositoryItem == nullptr)
+            {
+                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(requiredObjects, [&](size_t i) {
-            auto& otl = requiredObjects[i];
-            auto* requiredObject = otl.RepositoryItem;
-            if (requiredObject != nullptr)
+        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<std::mutex> guard(commonMutex);
+            if (newObject == 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<std::mutex> 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<std::mutex> guard(commonMutex);
-                    objects.push_back(loadedObject);
-                }
+                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)
         {