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

fix: handle Queue::submit non-fatally #6318

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Sep 23, 2024

Connections

Fixes #6317, unblocks #5714.

Description

  • Change the signature of wgpu_core::Global::queue_submit to return a (SubmissionIndex, …) in addition to its current error type.
  • Change the control flow of errors in Queue::submit to break to the end of a block. This is similar to what we already do in many APIs in wgpu_core.
  • Hoist the scope of the local submit_index binding so it can be used at the point where we need to convert current error paths to also return the submission index.

Later, we will likely want to avoid actually retrieving a new submission index so we can minimize the critical section of code. We'll need to figure out a strategy for returning a valid (but not necessarily unique) index in the case of failures that prevent successful submission.

It is strongly recommended that a reviewer review this diff with whitespace changes ignored.

Testing

Exercised by #6317. If a reviewer feels strongly, we can take the example from #5714 and turn it into a test.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file. I don't think that this is necessary, since we haven't shipped this defect to Crates.io.

@ErichDonGubler ErichDonGubler self-assigned this Sep 23, 2024
@ErichDonGubler ErichDonGubler added area: validation Issues related to validation, diagnostics, and error handling area: correctness We're behaving incorrectly labels Sep 23, 2024
@cwfitzgerald

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-lvknmkzzvkqt branch 4 times, most recently from e1de5f4 to 4359575 Compare September 24, 2024 15:02
@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler marked this pull request as ready for review September 24, 2024 15:02
@ErichDonGubler ErichDonGubler requested review from crowlKats and a team as code owners September 24, 2024 15:02
@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-lvknmkzzvkqt branch from 4359575 to 6f4f701 Compare September 24, 2024 15:11
wgpu-core/src/device/queue.rs Outdated Show resolved Hide resolved
* Change the signature of `wgpu_core::Global::queue_submit` to return
  a `(SubmissionIndex, …)` in addition to its current error type.
* Change the control flow of errors in `Queue::submit` to break to the
  end of a block. This is similar to what we already do in many APIs in
  `wgpu_core`.
* Hoist the scope of the local `submit_index` binding so it can be used
  at the point where we need to convert current error paths to also
  return the submission index.

Later, we will likely want to avoid actually retrieving a new submission
index so we can minimize the critical section of code. We'll need to
figure out a strategy for returning a valid (but not necessarily unique)
index in the case of failures that prevent successful submission.
@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-lvknmkzzvkqt branch from 6f4f701 to 816c139 Compare September 25, 2024 02:41
@ErichDonGubler ErichDonGubler enabled auto-merge (squash) September 25, 2024 02:47
@ErichDonGubler ErichDonGubler merged commit fc2fd95 into gfx-rs:trunk Sep 25, 2024
27 checks passed
@ErichDonGubler ErichDonGubler deleted the erichdongubler/push-lvknmkzzvkqt branch September 25, 2024 02:52
@cwfitzgerald
Copy link
Member

Nice, just started to need this in #6291 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: correctness We're behaving incorrectly area: validation Issues related to validation, diagnostics, and error handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queue::submit should not panic on validation errors
2 participants