Convert map_async from being async to being callback based

deno_webgpu/src/buffer.rs

@@ -83,36 +83,27 @@ pub async fn op_webgpu_buffer_get_map_async(
             .get::<super::WebGpuDevice>(device_rid)?;
         device = device_resource.0;
 
-        let boxed_sender = Box::new(sender);
-        let sender_ptr = Box::into_raw(boxed_sender) as *mut u8;
-
-        extern "C" fn buffer_map_future_wrapper(
-            status: wgpu_core::resource::BufferMapAsyncStatus,
-            user_data: *mut u8,
-        ) {
-            let sender_ptr = user_data as *mut oneshot::Sender<Result<(), AnyError>>;
-            let boxed_sender = unsafe { Box::from_raw(sender_ptr) };
-            boxed_sender
+        let callback = Box::new(move |status| {
+            sender
                 .send(match status {
                     wgpu_core::resource::BufferMapAsyncStatus::Success => Ok(()),
                     _ => unreachable!(), // TODO
                 })
                 .unwrap();
-        }
+        });
 
         // TODO(lucacasonato): error handling
         let maybe_err = gfx_select!(buffer => instance.buffer_map_async(
-          buffer,
-          offset..(offset + size),
-          wgpu_core::resource::BufferMapOperation {
-            host: match mode {
-              1 => wgpu_core::device::HostMap::Read,
-              2 => wgpu_core::device::HostMap::Write,
-              _ => unreachable!(),
-            },
-            callback: buffer_map_future_wrapper,
-            user_data: sender_ptr,
-          }
+            buffer,
+            offset..(offset + size),
+            wgpu_core::resource::BufferMapOperation {
+                host: match mode {
+                    1 => wgpu_core::device::HostMap::Read,
+                    2 => wgpu_core::device::HostMap::Write,
+                    _ => unreachable!(),
+                },
+                callback: wgpu_core::resource::BufferMapCallback::Rust { callback },
+            }
         ))
         .err();
 

player/tests/test.rs

@@ -14,7 +14,7 @@ use std::{
     fs::{read_to_string, File},
     io::{Read, Seek, SeekFrom},
     path::{Path, PathBuf},
-    ptr, slice,
+    slice,
 };
 
 #[derive(serde::Deserialize)]
@@ -55,7 +55,7 @@ struct Test<'a> {
     actions: Vec<wgc::device::trace::Action<'a>>,
 }
 
-extern "C" fn map_callback(status: wgc::resource::BufferMapAsyncStatus, _user_data: *mut u8) {
+fn map_callback(status: wgc::resource::BufferMapAsyncStatus) {
     match status {
         wgc::resource::BufferMapAsyncStatus::Success => (),
         _ => panic!("Unable to map"),
@@ -112,8 +112,9 @@ impl Test<'_> {
                 expect.offset .. expect.offset+expect.data.len() as wgt::BufferAddress,
                 wgc::resource::BufferMapOperation {
                     host: wgc::device::HostMap::Read,
-                    callback: map_callback,
-                    user_data: ptr::null_mut(),
+                    callback: wgc::resource::BufferMapCallback::Rust {
+                        callback: Box::new(map_callback),
+                    },
                 }
             ))
             .unwrap();

wgpu-core/src/device/life.rs

@@ -440,15 +440,18 @@ impl<A: hal::Api> LifetimeTracker<A> {
         }
     }
 
-    pub fn add_work_done_closure(&mut self, closure: SubmittedWorkDoneClosure) -> bool {
+    pub fn add_work_done_closure(
+        &mut self,
+        closure: SubmittedWorkDoneClosure,
+    ) -> Option<SubmittedWorkDoneClosure> {
         match self.active.last_mut() {
             Some(active) => {
                 active.work_done_closures.push(closure);
-                true
+                None
             }
             // Note: we can't immediately invoke the closure, since it assumes
             // nothing is currently locked in the hubs.
-            None => false,
+            None => Some(closure),
         }
     }
 }

wgpu-core/src/device/mod.rs

@@ -141,14 +141,14 @@ impl UserClosures {
         self.submissions.extend(other.submissions);
     }
 
-    unsafe fn fire(self) {
-        //Note: this logic is specifically moved out of `handle_mapping()` in order to
+    fn fire(self) {
+        // Note: this logic is specifically moved out of `handle_mapping()` in order to
         // have nothing locked by the time we execute users callback code.
         for (operation, status) in self.mappings {
-            (operation.callback)(status, operation.user_data);
+            operation.callback.call(status);
         }
         for closure in self.submissions {
-            (closure.callback)(closure.user_data);
+            closure.call();
         }
     }
 }
@@ -4978,9 +4978,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 .map_err(|_| DeviceError::Invalid)?
                 .maintain(hub, force_wait, &mut token)?
         };
-        unsafe {
-            closures.fire();
-        }
+
+        closures.fire();
+
         Ok(queue_empty)
     }
 
@@ -5058,9 +5058,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 self.poll_devices::<hal::api::Gles>(force_wait, &mut closures)? && all_queue_empty;
         }
 
-        unsafe {
-            closures.fire();
-        }
+        closures.fire();
 
         Ok(all_queue_empty)
     }
@@ -5167,7 +5165,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                     return Err(resource::BufferAccessError::AlreadyMapped);
                 }
                 resource::BufferMapState::Waiting(_) => {
-                    op.call_error();
+                    op.callback.call_error();
                     return Ok(());
                 }
                 resource::BufferMapState::Idle => {
@@ -5384,9 +5382,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         //Note: outside inner function so no locks are held when calling the callback
         let closure = self.buffer_unmap_inner::<A>(buffer_id)?;
         if let Some((operation, status)) = closure {
-            unsafe {
-                (operation.callback)(status, operation.user_data);
-            }
+            operation.callback.call(status);
         }
         Ok(())
     }

wgpu-core/src/device/queue.rs

@@ -28,16 +28,56 @@ use thiserror::Error;
 /// without a concrete moment of when it can be cleared.
 const WRITE_COMMAND_BUFFERS_PER_POOL: usize = 64;
 
-pub type OnSubmittedWorkDoneCallback = unsafe extern "C" fn(user_data: *mut u8);
 #[repr(C)]
-#[derive(Clone, Copy, Debug)]
+pub struct SubmittedWorkDoneClosureC {
+    callback: unsafe extern "C" fn(user_data: *mut u8),
+    user_data: *mut u8,
+}
+
+unsafe impl Send for SubmittedWorkDoneClosureC {}
+
 pub struct SubmittedWorkDoneClosure {
-    pub callback: OnSubmittedWorkDoneCallback,
-    pub user_data: *mut u8,
+    // We wrap this so creating the enum in the C variant can be unsafe,
+    // allowing our call function to be safe.
+    inner: SubmittedWorkDoneClosureInner,
+}
+
+enum SubmittedWorkDoneClosureInner {
+    Rust {
+        callback: Box<dyn FnOnce() + Send + 'static>,
+    },
+    C {
+        inner: SubmittedWorkDoneClosureC,
+    },
 }
 
-unsafe impl Send for SubmittedWorkDoneClosure {}
-unsafe impl Sync for SubmittedWorkDoneClosure {}
+impl SubmittedWorkDoneClosure {
+    pub fn from_rust(callback: Box<dyn FnOnce() + Send + 'static>) -> Self {
+        Self {
+            inner: SubmittedWorkDoneClosureInner::Rust { callback },
+        }
+    }
+
+    /// # Safety
+    ///
+    /// - The callback pointer must be valid to call with the provided user_data pointer.
+    /// - Both pointers must point to 'static data as the callback may happen at an unspecified time.
+    pub unsafe fn from_c(inner: SubmittedWorkDoneClosureC) -> Self {
+        Self {
+            inner: SubmittedWorkDoneClosureInner::C { inner },
+        }
+    }
+
+    pub(crate) fn call(self) {
+        match self.inner {
+            SubmittedWorkDoneClosureInner::Rust { callback } => callback(),
+            // SAFETY: the contract of the call to from_c says that this unsafe is sound.
+            SubmittedWorkDoneClosureInner::C { inner } => unsafe {
+                (inner.callback)(inner.user_data)
+            },
+        }
+    }
+}
 
 struct StagingData<A: hal::Api> {
     buffer: A::Buffer,
@@ -932,9 +972,8 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         };
 
         // the closures should execute with nothing locked!
-        unsafe {
-            callbacks.fire();
-        }
+        callbacks.fire();
+
         Ok(())
     }
 
@@ -957,7 +996,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         closure: SubmittedWorkDoneClosure,
     ) -> Result<(), InvalidQueue> {
         //TODO: flush pending writes
-        let added = {
+        let closure_opt = {
             let hub = A::hub(self);
             let mut token = Token::root();
             let (device_guard, mut token) = hub.devices.read(&mut token);
@@ -966,10 +1005,8 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 Err(_) => return Err(InvalidQueue),
             }
         };
-        if !added {
-            unsafe {
-                (closure.callback)(closure.user_data);
-            }
+        if let Some(closure) = closure_opt {
+            closure.call();
         }
         Ok(())
     }

wgpu-core/src/resource.rs

@@ -23,7 +23,6 @@ pub enum BufferMapAsyncStatus {
     ContextLost,
 }
 
-#[derive(Debug)]
 pub(crate) enum BufferMapState<A: hal::Api> {
     /// Mapped at creation.
     Init {
@@ -46,29 +45,67 @@ pub(crate) enum BufferMapState<A: hal::Api> {
 unsafe impl<A: hal::Api> Send for BufferMapState<A> {}
 unsafe impl<A: hal::Api> Sync for BufferMapState<A> {}
 
-pub type BufferMapCallback = unsafe extern "C" fn(status: BufferMapAsyncStatus, userdata: *mut u8);
-
 #[repr(C)]
-#[derive(Debug)]
-pub struct BufferMapOperation {
-    pub host: HostMap,
-    pub callback: BufferMapCallback,
-    pub user_data: *mut u8,
+pub struct BufferMapCallbackC {
+    callback: unsafe extern "C" fn(status: BufferMapAsyncStatus, user_data: *mut u8),
+    user_data: *mut u8,
 }
 
-//TODO: clarify if/why this is needed here
-unsafe impl Send for BufferMapOperation {}
-unsafe impl Sync for BufferMapOperation {}
+unsafe impl Send for BufferMapCallbackC {}
+
+pub struct BufferMapCallback {
+    // We wrap this so creating the enum in the C variant can be unsafe,
+    // allowing our call function to be safe.
+    inner: BufferMapCallbackInner,
+}
+
+enum BufferMapCallbackInner {
+    Rust {
+        callback: Box<dyn FnOnce(BufferMapAsyncStatus) + Send + 'static>,
+    },
+    C {
+        inner: BufferMapCallbackC,
+    },
+}
+
+impl BufferMapCallback {
+    pub fn from_rust(callback: Box<dyn FnOnce(BufferMapAsyncStatus) + Send + 'static>) -> Self {
+        Self {
+            inner: BufferMapCallbackInner::Rust { callback },
+        }
+    }
+
+    /// # Safety
+    ///
+    /// - The callback pointer must be valid to call with the provided user_data pointer.
+    /// - Both pointers must point to 'static data as the callback may happen at an unspecified time.
+    pub unsafe fn from_c(inner: BufferMapCallbackC) -> Self {
+        Self {
+            inner: BufferMapCallbackInner::C { inner },
+        }
+    }
+
+    pub(crate) fn call(self, status: BufferMapAsyncStatus) {
+        match self.inner {
+            BufferMapCallbackInner::Rust { callback } => callback(status),
+            // SAFETY: the contract of the call to from_c says that this unsafe is sound.
+            BufferMapCallbackInner::C { inner } => unsafe {
+                (inner.callback)(status, inner.user_data)
+            },
+        }
+    }
 
-impl BufferMapOperation {
     pub(crate) fn call_error(self) {
         log::error!("wgpu_buffer_map_async failed: buffer mapping is pending");
-        unsafe {
-            (self.callback)(BufferMapAsyncStatus::Error, self.user_data);
-        }
+        self.call(BufferMapAsyncStatus::Error);
     }
 }
 
+pub struct BufferMapOperation {
+    pub host: HostMap,
+    pub callback: BufferMapCallback,
+}
+
 #[derive(Clone, Debug, Error)]
 pub enum BufferAccessError {
     #[error(transparent)]
@@ -105,7 +142,6 @@ pub enum BufferAccessError {
     },
 }
 
-#[derive(Debug)]
 pub(crate) struct BufferPendingMapping {
     pub range: Range<wgt::BufferAddress>,
     pub op: BufferMapOperation,
@@ -115,7 +151,6 @@ pub(crate) struct BufferPendingMapping {
 
 pub type BufferDescriptor<'a> = wgt::BufferDescriptor<Label<'a>>;
 
-#[derive(Debug)]
 pub struct Buffer<A: hal::Api> {
     pub(crate) raw: Option<A::Buffer>,
     pub(crate) device_id: Stored<DeviceId>,