Fix race when recycling descriptor sets

CMakeLists.txt

@@ -242,4 +242,10 @@ vsg_add_option_maintainer(
 #
 add_subdirectory(src/vsg)
 
+option(VSG_BUILD_TESTS "Build VulkanSceneGraph tests" OFF)
+if (VSG_BUILD_TESTS)
+    enable_testing()
+    add_subdirectory(tests)
+endif()
+
 vsg_add_feature_summary()

src/vsg/vk/DescriptorPool.cpp

@@ -138,14 +138,11 @@ ref_ptr<DescriptorSet::Implementation> DescriptorPool::allocateDescriptorSet(Des
 
 void DescriptorPool::freeDescriptorSet(ref_ptr<DescriptorSet::Implementation> dsi)
 {
-    {
-        // swap ownership so that DescriptorSet::Implementation' reference is reset to null and while this DescriptorPool takes a reference to it.
-        // acquire lock within local scope so that subsequent dsi->_descriptorPool = {} call doesn't unref and (possibly) delete this DescriptorPool while lock still held.
-        std::scoped_lock<std::mutex> lock(mutex);
-        _recyclingList.push_back(dsi);
-        ++_availableDescriptorSet;
-        accumulateBindings(_recycledDescriptorPoolSizes, dsi->_descriptorSetLayout, +1);
-    }
+    ref_ptr<DescriptorPool> keepAlive(this);
+    std::scoped_lock<std::mutex> lock(mutex);
+    _recyclingList.push_back(dsi);
+    ++_availableDescriptorSet;
+    accumulateBindings(_recycledDescriptorPoolSizes, dsi->_descriptorSetLayout, +1);
     dsi->_descriptorPool = {};
 }
 

tests/CMakeLists.txt

@@ -0,0 +1,4 @@
+add_executable(vsg_descriptor_pool_recycle DescriptorPoolRecycle.cpp)
+target_link_libraries(vsg_descriptor_pool_recycle PRIVATE vsg::vsg)
+add_test(NAME vsg_descriptor_pool_recycle COMMAND vsg_descriptor_pool_recycle)
+set_tests_properties(vsg_descriptor_pool_recycle PROPERTIES SKIP_RETURN_CODE 77)

tests/DescriptorPoolRecycle.cpp

@@ -0,0 +1,63 @@
+#include <vsg/vk/Context.h>
+#include <vsg/vk/Instance.h>
+#include <vsg/vk/PhysicalDevice.h>
+
+#include <atomic>
+#include <chrono>
+#include <iostream>
+#include <thread>
+#include <utility>
+
+int main()
+{
+    auto instance = vsg::Instance::create(vsg::Names{}, vsg::Names{});
+    auto physicalDevice = instance->getPhysicalDevice(VK_QUEUE_GRAPHICS_BIT);
+    if (!physicalDevice)
+    {
+        std::cerr << "No Vulkan graphics device is available\n";
+        return 77;
+    }
+
+    int queueFamily = physicalDevice->getQueueFamily(VK_QUEUE_GRAPHICS_BIT);
+    auto device = vsg::Device::create(physicalDevice, vsg::QueueSettings{{queueFamily, {1.0f}}}, vsg::Names{}, vsg::Names{});
+    auto context = vsg::Context::create(device);
+    auto layout = vsg::DescriptorSetLayout::create();
+    layout->addBinding(0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_VERTEX_BIT);
+    layout->compile(*context);
+
+    auto pool = vsg::DescriptorPool::create(device, 1, vsg::DescriptorPoolSizes{{VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1}});
+    auto current = pool->allocateDescriptorSet(layout);
+    if (!current)
+    {
+        std::cerr << "Initial descriptor set allocation failed\n";
+        return 1;
+    }
+
+    for (int iteration = 0; iteration < 5000; ++iteration)
+    {
+        auto previous = std::move(current);
+        std::atomic<bool> recyclingStarted{false};
+
+        std::thread allocationThread([&] {
+            while (!recyclingStarted.load(std::memory_order_acquire)) std::this_thread::yield();
+            auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2);
+            while (!(current = pool->allocateDescriptorSet(layout)) && std::chrono::steady_clock::now() < deadline)
+                std::this_thread::yield();
+        });
+
+        std::thread recyclingThread([&] {
+            recyclingStarted.store(true, std::memory_order_release);
+            vsg::DescriptorSet::Implementation::recycle(previous);
+        });
+
+        recyclingThread.join();
+        allocationThread.join();
+        if (!current)
+        {
+            std::cerr << "Descriptor set reuse failed at iteration " << iteration << '\n';
+            return 1;
+        }
+    }
+
+    return 0;
+}