Skip to content

Commit

Permalink
Make DeviceLostClosure.from_c consume the closure before dropping it.
Browse files Browse the repository at this point in the history
This clarifies that the Rust and C-style callbacks/closures need to be
consumed (not called) before they are dropped. It also makes the from_c
function consume the param closure so that it can be dropped without
panicking.

It also relaxes the restriction that the callback/closure can only be
called once.
  • Loading branch information
bradwerth committed Jan 12, 2024
1 parent a24bdba commit 4e16f37
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ Passing an owned value `window` to `Surface` will return a `Surface<'static>`. S
- `BufferMappedRange` trait is now `WasmNotSendSync`, i.e. it is `Send`/`Sync` if not on wasm or `fragile-send-sync-non-atomic-wasm` is enabled. By @wumpf in [#4818](https://github.com/gfx-rs/wgpu/pull/4818)
- Align `wgpu_types::CompositeAlphaMode` serde serialization to spec. By @littledivy in [#4940](https://github.com/gfx-rs/wgpu/pull/4940)
- Fix error message of `ConfigureSurfaceError::TooLarge`. By @Dinnerbone in [#4960](https://github.com/gfx-rs/wgpu/pull/4960)
- Fix dropping of `DeviceLostCallbackC` params. By @bradwerth in [#5032](https://github.com/gfx-rs/wgpu/pull/5032)

#### DX12

Expand Down
34 changes: 16 additions & 18 deletions wgpu-core/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,13 @@ pub type DeviceLostCallback = Box<dyn Fn(DeviceLostReason, String) + 'static>;

pub struct DeviceLostClosureRust {
pub callback: DeviceLostCallback,
called: bool,
consumed: bool,
}

impl Drop for DeviceLostClosureRust {
fn drop(&mut self) {
if !self.called {
panic!("DeviceLostClosureRust must be called before it is dropped.");
if !self.consumed {
panic!("DeviceLostClosureRust must be consumed before it is dropped.");
}
}
}
Expand All @@ -239,7 +239,7 @@ impl Drop for DeviceLostClosureRust {
pub struct DeviceLostClosureC {
pub callback: unsafe extern "C" fn(user_data: *mut u8, reason: u8, message: *const c_char),
pub user_data: *mut u8,
called: bool,
consumed: bool,
}

#[cfg(any(
Expand All @@ -253,8 +253,8 @@ unsafe impl Send for DeviceLostClosureC {}

impl Drop for DeviceLostClosureC {
fn drop(&mut self) {
if !self.called {
panic!("DeviceLostClosureC must be called before it is dropped.");
if !self.consumed {
panic!("DeviceLostClosureC must be consumed before it is dropped.");
}
}
}
Expand All @@ -280,7 +280,7 @@ impl DeviceLostClosure {
pub fn from_rust(callback: DeviceLostCallback) -> Self {
let inner = DeviceLostClosureRust {
callback,
called: false,
consumed: false,
};
Self {
inner: DeviceLostClosureInner::Rust { inner },
Expand All @@ -294,14 +294,18 @@ impl DeviceLostClosure {
///
/// - Both pointers must point to `'static` data, as the callback may happen at
/// an unspecified time.
pub unsafe fn from_c(closure: DeviceLostClosureC) -> Self {
pub unsafe fn from_c(mut closure: DeviceLostClosureC) -> Self {
// Build an inner with the values from closure, ensuring that
// inner.called is false.
// inner.consumed is false.
let inner = DeviceLostClosureC {
callback: closure.callback,
user_data: closure.user_data,
called: false,
consumed: false,
};

// Mark the original closure as consumed, so we can safely drop it.
closure.consumed = true;

Self {
inner: DeviceLostClosureInner::C { inner },
}
Expand All @@ -310,19 +314,13 @@ impl DeviceLostClosure {
pub(crate) fn call(self, reason: DeviceLostReason, message: String) {
match self.inner {
DeviceLostClosureInner::Rust { mut inner } => {
if inner.called {
panic!("DeviceLostClosureRust must only be called once.");
}
inner.called = true;
inner.consumed = true;

(inner.callback)(reason, message)
}
// SAFETY: the contract of the call to from_c says that this unsafe is sound.
DeviceLostClosureInner::C { mut inner } => unsafe {
if inner.called {
panic!("DeviceLostClosureC must only be called once.");
}
inner.called = true;
inner.consumed = true;

// Ensure message is structured as a null-terminated C string. It only
// needs to live as long as the callback invocation.
Expand Down

0 comments on commit 4e16f37

Please sign in to comment.