Skip to content

Commit 7986803

Browse files
committed
RustFuture: Clarify Send bound and switch to RawWaker
Updated the `unsafe impl `Send` for WrappedFuture` docstring. I believe the new docs describe the situation more accurately. Moved back to using `RawWaker` because I want to drop the need for WrappedFuture to be `Send`. `WrappedFuture` will not always be `Send` when `wasm-unstable-single-threaded` is enabled. The current waker creation depends on `RustFuture` being `Send` which depends on `WrappedFuture` being `Send` (https://doc.rust-lang.org/std/task/struct.Waker.html#impl-From%3CArc%3CW%3E%3E-for-Waker). As a side bonus, I think the tracing printouts will improve.
1 parent f96887d commit 7986803

File tree

1 file changed

+72
-16
lines changed

1 file changed

+72
-16
lines changed

uniffi_core/src/ffi/rustfuture/future.rs

Lines changed: 72 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,10 @@
7878
use std::{
7979
future::Future,
8080
marker::PhantomData,
81-
ops::Deref,
8281
panic,
8382
pin::Pin,
8483
sync::{Arc, Mutex},
85-
task::{Context, Poll, Wake},
84+
task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
8685
};
8786

8887
use super::{
@@ -180,10 +179,16 @@ where
180179
}
181180
}
182181

183-
// If F and T are Send, then WrappedFuture is too
182+
// Implement `Send` on WrappedFuture under the correct conditions.
184183
//
185-
// Rust will not mark it Send by default when T::ReturnType is a raw pointer. This is promising
186-
// that we will treat the raw pointer properly, for example by not returning it twice.
184+
// FIXME: This is required to make the futures code compile, although it's not entirely correct.
185+
// If the `wasm-unstable-single-threaded` feature is enabled, then the wrapped future may not be `Send`, and thus `WrappedFuture` is not Send either.
186+
// This seems okay for now, since if `wasm-unstable-single-threaded` then futures should stay on one thread.
187+
// However, we should probably fix this in the long-term.
188+
//
189+
// In addition, `WrappedFuture::result` can be a raw pointer which is `!Send`.
190+
// This `!Send` bound is because Rust can't uphold safety guarantees for raw pointers.
191+
// However, this code does in fact treat the pointers correctly, for example we only return them across the FFI once.
187192
unsafe impl<T, UT> Send for WrappedFuture<T, UT>
188193
where
189194
T: FutureLowerReturn<UT> + 'static,
@@ -223,7 +228,7 @@ where
223228
let cancelled = self.is_cancelled();
224229
let ready = cancelled || {
225230
let mut locked = self.future.lock().unwrap();
226-
let waker: std::task::Waker = Arc::clone(&self).into();
231+
let waker = Arc::clone(&self).into_waker();
227232
locked.poll(&mut Context::from_waker(&waker))
228233
};
229234
if ready {
@@ -238,11 +243,6 @@ where
238243
self.scheduler.lock().unwrap().is_cancelled()
239244
}
240245

241-
pub(super) fn wake(&self) {
242-
trace!("RustFuture::wake called");
243-
self.scheduler.lock().unwrap().wake();
244-
}
245-
246246
pub(super) fn cancel(&self) {
247247
self.scheduler.lock().unwrap().cancel();
248248
}
@@ -259,17 +259,73 @@ where
259259
}
260260
}
261261

262-
impl<T, UT> Wake for RustFuture<T, UT>
262+
// `RawWaker` implementation for RustFuture
263+
impl<T, UT> RustFuture<T, UT>
263264
where
264265
T: FutureLowerReturn<UT> + 'static,
265266
UT: Send + 'static,
267+
Scheduler: Send + Sync,
266268
{
267-
fn wake(self: Arc<Self>) {
268-
self.deref().wake()
269+
unsafe fn waker_clone(ptr: *const ()) -> RawWaker {
270+
trace!("RustFuture::waker_clone called ({ptr:?)");
271+
Arc::<Self>::increment_strong_count(ptr.cast::<Self>());
272+
RawWaker::new(
273+
ptr,
274+
&RawWakerVTable::new(
275+
Self::waker_clone,
276+
Self::waker_wake,
277+
Self::waker_wake_by_ref,
278+
Self::waker_drop,
279+
),
280+
)
269281
}
270282

271-
fn wake_by_ref(self: &Arc<Self>) {
272-
self.deref().wake()
283+
unsafe fn waker_wake(ptr: *const ()) {
284+
trace!("RustFuture::waker_wake called ({ptr:?)");
285+
Self::recreate_arc(ptr).scheduler.lock().unwrap().wake();
286+
}
287+
288+
unsafe fn waker_wake_by_ref(ptr: *const ()) {
289+
trace!("RustFuture::waker_wake_by_ref called ({ptr:?)");
290+
// For wake_by_ref, we can use the pointer directly, without consuming it to re-create the
291+
// arc.
292+
let ptr = ptr.cast::<Self>();
293+
(*ptr).scheduler.lock().unwrap().wake();
294+
}
295+
296+
unsafe fn waker_drop(ptr: *const ()) {
297+
trace!("RustFuture::waker_drop called ({ptr:?)");
298+
drop(Self::recreate_arc(ptr));
299+
}
300+
301+
/// Recreate an Arc<Self> from a raw pointer
302+
///
303+
/// # Safety
304+
/// * ptr must have been created from `Arc::into_raw()`
305+
/// * ptr must only be used once to re-create the arc
306+
unsafe fn recreate_arc(ptr: *const ()) -> Arc<Self> {
307+
let ptr = ptr.cast::<Self>();
308+
Arc::<Self>::from_raw(ptr)
309+
}
310+
311+
fn into_waker(self: Arc<Self>) -> Waker {
312+
trace!("RustFuture::creating waker ({:?)", self.as_pointer());
313+
let raw_waker = RawWaker::new(
314+
Arc::into_raw(self).cast::<()>(),
315+
&RawWakerVTable::new(
316+
Self::waker_clone,
317+
Self::waker_wake,
318+
Self::waker_wake_by_ref,
319+
Self::waker_drop,
320+
),
321+
);
322+
323+
// Safety:
324+
//
325+
// * The vtable functions are thread-safe, since we only access `Self::scheduler` and that
326+
// has a `Send` + `Sync` bound.
327+
// * We manage the raw arc pointers correctly
328+
unsafe { Waker::from_raw(raw_waker) }
273329
}
274330
}
275331

0 commit comments

Comments
 (0)