CommandEncoder::finish sometimes panics unexpectedly when ComputePass is not explicitly dropped beforehand

[melvdlin]

Description
Borrow shortening can cause CommandEncoder::finish to be callable before a derived ComputePass is automatically dropped. Since the Drop impl on ComputePassInner is what unlocks the CommandEncoder, this leads to an unexpected validation error, which can be avoided by explicitly dropping of the ComputePass before calling CommandEncoder::finish.

The same likely applies to RenderPass.

Repro steps

fn main() {
    let instance = wgpu::Instance::new(Default::default());
    let adapter =
        pollster::block_on(instance.request_adapter(&Default::default())).unwrap();
    let (device, queue) =
        pollster::block_on(adapter.request_device(&Default::default(), None)).unwrap();

    let mut encoder = device.create_command_encoder(&Default::default());
    let compute_pass = encoder.begin_compute_pass(&Default::default());
    // set up the compute pass...

    let cmd_buffer = encoder.finish();
    assert!(device
        .poll(wgpu::Maintain::wait_for(queue.submit([cmd_buffer])))
        .is_queue_empty());
}

[dependencies]
pollster = "0.3.0"
wgpu = "22.1.0"

Expected vs observed behavior
A panic occurs at line 12:

wgpu error: Validation Error

Caused by:
  In a CommandEncoder
    Command encoder is locked by a previously created render/compute pass. Before recording any new commands, the pass must be ended.

Since ComputePass's lifetime is bounded by a mutable borrow of the CommandEncoder, one would expect the CommandEncoder to be unlocked and a call to finish to be valid as soon as said borrow is invalidated. This is, however, not the case, since the Drop impl of ComputePassInner is only run at the end of scope, not as soon as the borrow is invalidated.

The documentation of CommandEncoder::begin_compute_pass does contain the following line: As long as the returned ComputePass has not ended, any mutating operation on this command encoder causes an error and invalidates it.. It does not specify that "a compute pass ending" specifically means "the ComputePass being dropped".

Platform
wgpu = "22.1.0"

[kpreid]

If the cause is exactly as described, perhaps a simple fix for this would be to add:

impl Drop for ComputePass { fn drop(&mut self) {} }
impl Drop for RenderPass { fn drop(&mut self) {} }

This should require the lifetime to be valid when the pass is dropped.

[ErichDonGubler]

I can confirm the crash on my Win11 machine. The workaround typically shown in examples right now is to scope the compute_pass so that it's dropped before calling encoder.finish():

--- a/src/main.rs
+++ b/src/main.rs
@@ -5,8 +5,10 @@
         pollster::block_on(adapter.request_device(&Default::default(), None)).unwrap();
 
     let mut encoder = device.create_command_encoder(&Default::default());
-    let compute_pass = encoder.begin_compute_pass(&Default::default());
-    // set up the compute pass...
+    {
+        let compute_pass = encoder.begin_compute_pass(&Default::default());
+        // set up the compute pass...
+    }
 
     let cmd_buffer = encoder.finish();
     assert!(device

…but, of course, this is still a paper cut we should strive to eliminate in our Rust-y interpretation of the WebGPU API.

[ErichDonGubler]

I interpret @kpreid's suggestion to mean a diff. like this against current trunk:

diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs
index eb929740c8..f216d1b40f 100644
--- a/wgpu-core/src/command/compute.rs
+++ b/wgpu-core/src/command/compute.rs
@@ -95,6 +95,10 @@
     }
 }
 
+impl Drop for ComputePass {
+    fn drop(&mut self) {}
+}
+
 #[derive(Clone, Debug, Default)]
 pub struct ComputePassDescriptor<'a> {
     pub label: Label<'a>,
diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs
index e4d93b042e..efc80d7475 100644
--- a/wgpu-core/src/command/render.rs
+++ b/wgpu-core/src/command/render.rs
@@ -321,6 +321,10 @@
     }
 }
 
+impl Drop for RenderPass {
+    fn drop(&mut self) {}
+}
+
 #[derive(Debug, PartialEq)]
 enum OptionalState {
     Unused,

…but this doesn't change any behavior in the MRE posted in the OP.

[teoxoy]

Render/compute passes no longer have lifetimes (#1453 was recently closed by @Wumpf's PRs).

We might want to expose the end/finish methods and require usage of those instead of implicitly doing it on drop.

[cwfitzgerald]

They do still have lifetimes to their encoder.

No, the RenderPass drop needs to be added in wgpu itself.

[cwfitzgerald]

https://github.com/gfx-rs/wgpu/blob/trunk/wgpu/src/api/render_pass.rs#L39

[teoxoy]

Ah, I see, I should have checked before writing that :)

[cwfitzgerald]

Going to tackle this as part of #6619. I need to write a test to make sure I didn't break anything anyway.