Prevent a deadlock when destroying resources

wgpu-core/src/device/global.rs

@@ -2107,6 +2107,12 @@ impl Global {
     ) -> Result<bool, WaitIdleError> {
         api_log!("Device::poll");
 
+        let hub = A::hub(self);
+        let device = hub
+            .devices
+            .get(device_id)
+            .map_err(|_| DeviceError::Invalid)?;
+
         let (closures, queue_empty) = {
             if let wgt::Maintain::WaitForSubmissionIndex(submission_index) = maintain {
                 if submission_index.queue_id != device_id.transmute() {
@@ -2117,16 +2123,15 @@ impl Global {
                 }
             }
 
-            let hub = A::hub(self);
-            let device = hub
-                .devices
-                .get(device_id)
-                .map_err(|_| DeviceError::Invalid)?;
             let fence = device.fence.read();
             let fence = fence.as_ref().unwrap();
             device.maintain(fence, maintain)?
         };
 
+        // Some deferred destroys are scheduled in maintain so run this right after
+        // to avoid holding on to them until the next device poll.
+        device.deferred_resource_destruction();
+
         closures.fire();
 
         Ok(queue_empty)

wgpu-core/src/device/resource.rs

@@ -1,12 +1,13 @@
 #[cfg(feature = "trace")]
 use crate::device::trace;
 use crate::{
-    binding_model::{self, BindGroupLayout, BindGroupLayoutEntryError},
+    binding_model::{self, BindGroup, BindGroupLayout, BindGroupLayoutEntryError},
     command, conv,
-    device::life::{LifetimeTracker, WaitIdleError},
-    device::queue::PendingWrites,
     device::{
-        bgl, AttachmentData, CommandAllocator, DeviceLostInvocation, MissingDownlevelFlags,
+        bgl,
+        life::{LifetimeTracker, WaitIdleError},
+        queue::PendingWrites,
+        AttachmentData, CommandAllocator, DeviceLostInvocation, MissingDownlevelFlags,
         MissingFeatures, RenderPassContext, CLEANUP_WAIT_MS,
     },
     hal_api::HalApi,
@@ -21,10 +22,9 @@ use crate::{
     pipeline,
     pool::ResourcePool,
     registry::Registry,
-    resource::ResourceInfo,
     resource::{
-        self, Buffer, QuerySet, Resource, ResourceType, Sampler, Texture, TextureView,
-        TextureViewNotRenderableReason,
+        self, Buffer, QuerySet, Resource, ResourceInfo, ResourceType, Sampler, Texture,
+        TextureView, TextureViewNotRenderableReason,
     },
     resource_log,
     snatch::{SnatchGuard, SnatchLock, Snatchable},
@@ -48,7 +48,7 @@ use std::{
     num::NonZeroU32,
     sync::{
         atomic::{AtomicBool, AtomicU64, Ordering},
-        Arc,
+        Arc, Weak,
     },
 };
 
@@ -129,10 +129,16 @@ pub struct Device<A: HalApi> {
     pub(crate) downlevel: wgt::DownlevelCapabilities,
     pub(crate) instance_flags: wgt::InstanceFlags,
     pub(crate) pending_writes: Mutex<Option<PendingWrites<A>>>,
+    pub(crate) deferred_destroy: Mutex<Vec<DeferredDestroy<A>>>,
     #[cfg(feature = "trace")]
     pub(crate) trace: Mutex<Option<trace::Trace>>,
 }
 
+pub(crate) enum DeferredDestroy<A: HalApi> {
+    TextureView(Weak<TextureView<A>>),
+    BindGroup(Weak<BindGroup<A>>),
+}
+
 impl<A: HalApi> std::fmt::Debug for Device<A> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("Device")
@@ -285,6 +291,7 @@ impl<A: HalApi> Device<A> {
             downlevel,
             instance_flags,
             pending_writes: Mutex::new(Some(pending_writes)),
+            deferred_destroy: Mutex::new(Vec::new()),
         })
     }
 
@@ -300,6 +307,56 @@ impl<A: HalApi> Device<A> {
         self.life_tracker.lock()
     }
 
+    /// Run some destroy operations that were deferred.
+    ///
+    /// Destroying the resources requires taking a write lock on the device's snatch lock,
+    /// so a good reason for deferring resource destruction is when we don't know for sure
+    /// how risky it is to take the lock (typically, it shouldn't be taken from the drop
+    /// implementation of a reference-counted structure).
+    /// The snatch lock must not be held while this function is called.
+    pub(crate) fn deferred_resource_destruction(&self) {
+        while let Some(item) = self.deferred_destroy.lock().pop() {
+            match item {
+                DeferredDestroy::TextureView(view) => {
+                    let Some(view) = view.upgrade() else {
+                        continue;
+                    };
+                    let Some(raw_view) = view.raw.snatch(self.snatchable_lock.write()) else {
+                        continue;
+                    };
+
+                    resource_log!("Destroy raw TextureView (destroyed) {:?}", view.label());
+                    #[cfg(feature = "trace")]
+                    if let Some(t) = self.trace.lock().as_mut() {
+                        t.add(trace::Action::DestroyTextureView(view.info.id()));
+                    }
+                    unsafe {
+                        use hal::Device;
+                        self.raw().destroy_texture_view(raw_view);
+                    }
+                }
+                DeferredDestroy::BindGroup(bind_group) => {
+                    let Some(bind_group) = bind_group.upgrade() else {
+                        continue;
+                    };
+                    let Some(raw_bind_group) = bind_group.raw.snatch(self.snatchable_lock.write()) else {
+                        continue;
+                    };
+
+                    resource_log!("Destroy raw BindGroup (destroyed) {:?}", bind_group.label());
+                    #[cfg(feature = "trace")]
+                    if let Some(t) = self.trace.lock().as_mut() {
+                        t.add(trace::Action::DestroyBindGroup(bind_group.info.id()));
+                    }
+                    unsafe {
+                        use hal::Device;
+                        self.raw().destroy_bind_group(raw_bind_group);
+                    }
+                }
+            }
+        }
+    }
+
     /// Check this device for completed commands.
     ///
     /// The `maintain` argument tells how the maintence function should behave, either
@@ -1972,7 +2029,7 @@ impl<A: HalApi> Device<A> {
         layout: &Arc<BindGroupLayout<A>>,
         desc: &binding_model::BindGroupDescriptor,
         hub: &Hub<A>,
-    ) -> Result<binding_model::BindGroup<A>, binding_model::CreateBindGroupError> {
+    ) -> Result<BindGroup<A>, binding_model::CreateBindGroupError> {
         use crate::binding_model::{BindingResource as Br, CreateBindGroupError as Error};
         {
             // Check that the number of entries in the descriptor matches
@@ -2212,7 +2269,7 @@ impl<A: HalApi> Device<A> {
                 .map_err(DeviceError::from)?
         };
 
-        Ok(binding_model::BindGroup {
+        Ok(BindGroup {
             raw: Snatchable::new(raw),
             device: self.clone(),
             layout: layout.clone(),

wgpu-core/src/resource.rs

@@ -3,8 +3,8 @@ use crate::device::trace;
 use crate::{
     binding_model::BindGroup,
     device::{
-        queue, BufferMapPendingClosure, Device, DeviceError, HostMap, MissingDownlevelFlags,
-        MissingFeatures,
+        queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError, HostMap,
+        MissingDownlevelFlags, MissingFeatures,
     },
     global::Global,
     hal_api::HalApi,
@@ -604,29 +604,6 @@ impl<A: HalApi> Resource for Buffer<A> {
     }
 }
 
-fn snatch_and_destroy_bind_groups<A: HalApi>(
-    device: &Device<A>,
-    bind_groups: &[Weak<BindGroup<A>>],
-) {
-    for bind_group in bind_groups {
-        if let Some(bind_group) = bind_group.upgrade() {
-            if let Some(raw_bind_group) = bind_group.raw.snatch(device.snatchable_lock.write()) {
-                resource_log!("Destroy raw BindGroup (destroyed) {:?}", bind_group.label());
-
-                #[cfg(feature = "trace")]
-                if let Some(t) = device.trace.lock().as_mut() {
-                    t.add(trace::Action::DestroyBindGroup(bind_group.info.id()));
-                }
-
-                unsafe {
-                    use hal::Device;
-                    device.raw().destroy_bind_group(raw_bind_group);
-                }
-            }
-        }
-    }
-}
-
 /// A buffer that has been marked as destroyed and is staged for actual deletion soon.
 #[derive(Debug)]
 pub struct DestroyedBuffer<A: HalApi> {
@@ -650,7 +627,11 @@ impl<A: HalApi> DestroyedBuffer<A> {
 
 impl<A: HalApi> Drop for DestroyedBuffer<A> {
     fn drop(&mut self) {
-        snatch_and_destroy_bind_groups(&self.device, &self.bind_groups);
+        let mut deferred = self.device.deferred_destroy.lock();
+        for bind_group in self.bind_groups.drain(..) {
+            deferred.push(DeferredDestroy::BindGroup(bind_group));
+        }
+        drop(deferred);
 
         if let Some(raw) = self.raw.take() {
             resource_log!("Destroy raw Buffer (destroyed) {:?}", self.label());
@@ -1038,25 +1019,16 @@ impl<A: HalApi> DestroyedTexture<A> {
 impl<A: HalApi> Drop for DestroyedTexture<A> {
     fn drop(&mut self) {
         let device = &self.device;
-        snatch_and_destroy_bind_groups(device, &self.bind_groups);
 
+        let mut deferred = device.deferred_destroy.lock();
         for view in self.views.drain(..) {
-            if let Some(view) = view.upgrade() {
-                if let Some(raw_view) = view.raw.snatch(device.snatchable_lock.write()) {
-                    resource_log!("Destroy raw TextureView (destroyed) {:?}", view.label());
-
-                    #[cfg(feature = "trace")]
-                    if let Some(t) = self.device.trace.lock().as_mut() {
-                        t.add(trace::Action::DestroyTextureView(view.info.id()));
-                    }
-
-                    unsafe {
-                        use hal::Device;
-                        self.device.raw().destroy_texture_view(raw_view);
-                    }
-                }
-            }
+            deferred.push(DeferredDestroy::TextureView(view));
         }
+        for bind_group in self.bind_groups.drain(..) {
+            deferred.push(DeferredDestroy::BindGroup(bind_group));
+        }
+        drop(deferred);
+
         if let Some(raw) = self.raw.take() {
             resource_log!("Destroy raw Texture (destroyed) {:?}", self.label());