Repository: gfx-rs/wgpu
Pull request #5475 (2024-04-02 00:27:05)

fix: unlock guard for `release_gpu_resources` call in `Device::maintain`
**Connections**

Resolves <https://github.com/gfx-rs/wgpu/issues/5471>.

**Description**

Resolves <https://github.com/gfx-rs/wgpu/issues/5471> by unlocking the snatch guard that conflicts with the call to `Device::release_gpu_resources` inside of `Device::maintain`.

**Testing**

Add the new test case `wgpu_test::poll_destroyed_device` which exercises the deadlock prior to this fix. You can test like so:

```
cargo nextest run --all-features -p wgpu-test -- wgpu_test::device::poll_destroyed_device
```

**Checklist**

- [x] Run `cargo fmt`.
- [x] Run `cargo clippy`. If applicable, add:
  - [x] `--target wasm32-unknown-unknown`
  - [x] `--target wasm32-unknown-emscripten`
- [x] Run `cargo xtask test` to run tests.
- [x] ~~Add change to `CHANGELOG.md`. See simple instructions inside file.~~ This fixes an unreleased fix with no `CHANGELOG` entry, so I don't think that this is necessary.


jimblandy: @ErichDonGubler I've pushed two commits:

- The first changes the test case. I adopted parts of yours that were clearer.
- The second takes a different approach to handling the snatchguard. I think this is simpler.

Please take a look and let me know what you think.

Patch:
diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs
index f52e9d0b6..0c97e1b50 100644
--- a/wgpu-core/src/device/global.rs
+++ b/wgpu-core/src/device/global.rs
@@ -2019,7 +2019,7 @@ impl Global {
                 let snatch_guard = device.snatchable_lock.read();
                 let fence = device.fence.read();
                 let fence = fence.as_ref().unwrap();
-                match device.maintain(fence, wgt::Maintain::Wait, &snatch_guard) {
+                match device.maintain(fence, wgt::Maintain::Wait, snatch_guard) {
                     Ok((closures, _)) => {
                         user_callbacks = closures;
                     }
@@ -2132,7 +2132,7 @@ impl Global {
         let snatch_guard = device.snatchable_lock.read();
         let fence = device.fence.read();
         let fence = fence.as_ref().unwrap();
-        let (closures, queue_empty) = device.maintain(fence, maintain, &snatch_guard)?;
+        let (closures, queue_empty) = device.maintain(fence, maintain, snatch_guard)?;
 
         // Some deferred destroys are scheduled in maintain so run this right after
         // to avoid holding on to them until the next device poll.
diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs
index ca63a6bb1..3cb5f695a 100644
--- a/wgpu-core/src/device/queue.rs
+++ b/wgpu-core/src/device/queue.rs
@@ -1545,7 +1545,7 @@ impl Global {
 
             // This will schedule destruction of all resources that are no longer needed
             // by the user but used in the command stream, among other things.
-            let (closures, _) = match device.maintain(fence, wgt::Maintain::Poll, &snatch_guard) {
+            let (closures, _) = match device.maintain(fence, wgt::Maintain::Poll, snatch_guard) {
                 Ok(closures) => closures,
                 Err(WaitIdleError::Device(err)) => return Err(QueueSubmitError::Queue(err)),
                 Err(WaitIdleError::StuckGpu) => return Err(QueueSubmitError::StuckGpu),
diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs
index 7de87c6fd..4892aecb7 100644
--- a/wgpu-core/src/device/resource.rs
+++ b/wgpu-core/src/device/resource.rs
@@ -394,7 +394,7 @@ impl<A: HalApi> Device<A> {
         &'this self,
         fence: &A::Fence,
         maintain: wgt::Maintain<queue::WrappedSubmissionIndex>,
-        snatch_guard: &SnatchGuard,
+        snatch_guard: SnatchGuard,
     ) -> Result<(UserClosures, bool), WaitIdleError> {
         profiling::scope!("Device::maintain");
         let last_done_index = if maintain.is_wait() {
@@ -449,7 +449,7 @@ impl<A: HalApi> Device<A> {
         }
 
         let mapping_closures =
-            life_tracker.handle_mapping(self.raw(), &self.trackers, snatch_guard);
+            life_tracker.handle_mapping(self.raw(), &self.trackers, &snatch_guard);
 
         let queue_empty = life_tracker.queue_empty();
 
@@ -476,8 +476,9 @@ impl<A: HalApi> Device<A> {
             }
         }
 
-        // Don't hold the lock while calling release_gpu_resources.
+        // Don't hold the locks while calling release_gpu_resources.
         drop(life_tracker);
+        drop(snatch_guard);
 
         if should_release_gpu_resource {
             self.release_gpu_resources();


Test patch:
diff --git a/tests/tests/device.rs b/tests/tests/device.rs
index fd393b63b..ff596d091 100644
--- a/tests/tests/device.rs
+++ b/tests/tests/device.rs
@@ -776,3 +776,42 @@ fn vs_main() -> @builtin(position) vec4<f32> {
             ],
         });
     });
+
+#[gpu_test]
+static DEVICE_DESTROY_THEN_BUFFER_CLEANUP: GpuTestConfiguration = GpuTestConfiguration::new()
+    .parameters(TestParameters::default())
+    .run_sync(|ctx| {
+        // When a device is destroyed, its resources should be released,
+        // without causing a deadlock.
+
+        // Create a buffer to be left around until the device is destroyed.
+        let _buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor {
+            label: None,
+            size: 256,
+            usage: wgpu::BufferUsages::MAP_READ,
+            mapped_at_creation: false,
+        });
+
+        // Create a texture to be left around until the device is destroyed.
+        let texture_extent = wgpu::Extent3d {
+            width: 512,
+            height: 512,
+            depth_or_array_layers: 1,
+        };
+        let _texture = ctx.device.create_texture(&wgpu::TextureDescriptor {
+            label: None,
+            size: texture_extent,
+            mip_level_count: 2,
+            sample_count: 1,
+            dimension: wgpu::TextureDimension::D2,
+            format: wgpu::TextureFormat::Rg8Uint,
+            usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
+            view_formats: &[],
+        });
+
+        // Destroy the device.
+        ctx.device.destroy();
+
+        // Poll the device, which should try to clean up its resources.
+        ctx.instance.poll_all(true);
+    });