Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bundles still have lifetime constraints #6433

Open
fyellin opened this issue Oct 21, 2024 · 3 comments
Open

Bundles still have lifetime constraints #6433

fyellin opened this issue Oct 21, 2024 · 3 comments
Labels
area: api Issues related to API surface

Comments

@fyellin
Copy link

fyellin commented Oct 21, 2024

Although wgpu::RenderPass and wgpu::RenderBundleEncoder both implement the trait wpu::util::RenderEncoder, they have different expectations for the lifetime of their common methods. For methods such as set_pipeline, set_bind_group, and set_vertex_buffer, RenderPass has no expectations for the lifetime of the argument. RenderBundleEncoder requires that the argument live at least as long as self.

There is no way of enforcing this lifetime restriction in the C interface or the Python interface to this code. This is documented in this bug at wgpu-native.

Attached is RenderBundleEncoderBug.zip
which contains RenderBundleEncoder.py. It is run by calling python RenderBundleEncoderBug.py <flag> where for this bug, flag is one of null-pipeline, null-bind-group, null-vertex-buffer, null-index-buffer, null-indirect-buffer. These show that nulling any of these values has no affect on a RenderPass but will cause the RenderBundleEncoder to panic.

My preference is that this lifetime restriction in RenderBundleEncoder (and RenderEncoder) be removed. Whatever techniqueRenderPass uses to keep track of the objects (and keep them alive?) can certainly be applied to RenderBundleEncoder.

Alternatively, RenderBundleEncoder shouldn't panic. It should never panic except for a serious non-recoverable error. this is an error that can easily be reported back to the user.

@Wumpf Wumpf changed the title Inconsistency between RenderPass and RenderBundleEncoder Bundles still have lifetime constraints Oct 23, 2024
@Wumpf
Copy link
Member

Wumpf commented Oct 23, 2024

Those lifetime constraints got lifted for Render & Compute pass not too long ago, see #5884 and related PRs.
Very similar work has to be done for bundles. However, I thought that most of it was already in place. But obviously there's still something missing. A suite of tests similar to what was added for render & compute pass is in order, fixing those and then removing the those lifetimes from the public api

@fyellin
Copy link
Author

fyellin commented Oct 23, 2024

I recently wrote some test code for python that tests deleting objects in the user code even though they area still in use by wgpu. Everything worked perfectly except for RenderBundleEncoder. Behind the scenes, I had to force RenderBundleEncoder to retain a pointer to the objects it depended on.

Should I file a different bug about the fact that RenderBundle and RenderBundleEncoder panic when they find something they don't like, rather than returning an error? Or is this part of the same cleanup? Passing the wrong arguments formats to createRenderBundleEncoder() or forgetting to call set_pipeline() will cause a crash.

@Wumpf
Copy link
Member

Wumpf commented Oct 23, 2024

part of the same cleanup, I think this issue captures this well enough already.
Generally, violating the lifetime constraints that the rust borrow checker tries to uphold (and naturally can't be automatically enforced in a C interface) is undefined behavior. So it's less of a crash bug but more of a "those lifetimes really shouldn't be there" issue (:
(well and ofc from a different point of view, wgpu-native is currently not correctly implementing the contract of webgpu.h, which it could do easily if those lifetime constraints weren't there)

@Wumpf Wumpf added the area: api Issues related to API surface label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface
Projects
None yet
Development

No branches or pull requests

2 participants