Unverified Commit ba621e8a authored by James Hugman's avatar James Hugman
Browse files

Address reviewer comments

parent 156fe6c2
Loading
Loading
Loading
Loading
+8 −8
Original line number Diff line number Diff line
@@ -84,14 +84,14 @@ use std::{
    task::{Context, Poll, Wake},
};

use super::{FutureResult, RustFutureContinuationCallback, RustFuturePoll, Scheduler};
use super::{RustFutureContinuationCallback, RustFuturePoll, Scheduler, UniffiCompatibleFuture};
use crate::{rust_call_with_out_status, FfiDefault, LiftArgsError, LowerReturn, RustCallStatus};

/// Wraps the actual future we're polling
struct WrappedFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -105,7 +105,7 @@ where
impl<F, T, UT> WrappedFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -185,7 +185,7 @@ where
unsafe impl<F, T, UT> Send for WrappedFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -195,7 +195,7 @@ where
pub(super) struct RustFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -211,7 +211,7 @@ where
impl<F, T, UT> RustFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -266,7 +266,7 @@ where
impl<F, T, UT> Wake for RustFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
@@ -301,7 +301,7 @@ pub trait RustFutureFfi<ReturnType>: Send + Sync {
impl<F, T, UT> RustFutureFfi<T::ReturnType> for RustFuture<F, T, UT>
where
    // See rust_future_new for an explanation of these trait bounds
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    T: LowerReturn<UT> + Send + 'static,
    UT: Send + 'static,
{
+52 −7
Original line number Diff line number Diff line
@@ -30,16 +30,61 @@ pub enum RustFuturePoll {
/// to continue progress on the future.
pub type RustFutureContinuationCallback = extern "C" fn(callback_data: u64, RustFuturePoll);

/// This marker trait allows us to put different bounds on the `Future`s we support, based on
/// `#[cfg()]` configuration.
pub trait FutureResult<T, E>: Future<Output = Result<T, E>> + 'static {}
/// This marker trait allows us to put different bounds on the `Future`s we
/// support, based on `#[cfg(..)]` configuration.
///
/// It should not be considered as a part of the public API, and as such as
/// an implementation detail and subject to change.
///
/// It is _not_ intended to be implemented by libray users or bindings
/// implementors.
#[doc(hidden)]
pub trait UniffiCompatibleFuture<T>: Future<Output = T> {}

/// The `Send` bound is required because the Foreign code may call the
/// `rust_future_*` methods from different threads.
#[cfg(not(target_arch = "wasm32"))]
impl<T, F, E> FutureResult<T, E> for F where F: Future<Output = Result<T, E>> + Send + 'static {}
impl<T, F> UniffiCompatibleFuture<T> for F where F: Future<Output = T> + Send {}

// `Promise`s cannot be sent to or from WebWorkers, but `JsValue` is not `Send`.
/// `Future`'s on WASM32 are not `Send` because it's a single threaded environment.
///
/// # Safety:
///
/// WASM32 is a single threaded environment. However, in a browser there do
/// exist [`WebWorker`][webworker]s which do not share memory or event-loop
/// with the main browser context.
///
/// Communication between contexts is only possible by message passing,
/// using a small number of ['transferable' object types][transferable].
///
/// The most common source of asynchrony in Rust compiled to WASM is
/// [wasm-bindgen's `JsFuture`][jsfuture]. It is not `Send` because:
///
/// 1. `T` and `E` are both `JsValue`
/// 2. `JsValue` may contain `JsFunction`s, either as a function themselves or
///    an object containing functions.
/// 3. Functions cannot be [serialized and sent][transferable] to `WebWorker`s.
///
/// Implementors of binding generators should be able to enumerate the
/// combinations of Rust or JS communicating across different contexts (here
/// using: <->), and in the same context (+) to account for why it is safe
/// for UniFFI to support `Future`s that are not `Send`:
///
/// 1. JS + Rust in the same contexts: polling and waking happens in the same
///    thread, no `Send` is needed.
/// 2. Rust <-> Rust in different contexts: Futures cannot be sent between JS
///    contexts within the same Rust crate (because they are not `Send`).
/// 3. JS <-> Rust in different contexts: the `Promise` are [not transferable
///    between contexts][transferable], so this is impossible.
/// 4. JS <-> JS + Rust, this is possible, but safe since the Future is being
///    driven by JS in the same thread. If a Promise metaphor is desired, then
///    this must be built with JS talking to JS, because 3.
///
/// [webworker]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers
/// [jsfuture]: https://github.com/rustwasm/wasm-bindgen/blob/main/crates/futures/src/lib.rs
/// [transferable]: (https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Transferable_objects
#[cfg(target_arch = "wasm32")]
impl<T, F, E> FutureResult<T, E> for F where F: Future<Output = Result<T, E>> + 'static {}
impl<T, F> UniffiCompatibleFuture<T> for F where F: Future<Output = T> {}

// === Public FFI API ===

@@ -55,7 +100,7 @@ where
    // since it will move between threads for an indeterminate amount of time as the foreign
    // executor calls polls it and the Rust executor wakes it.  It does not need to by `Sync`,
    // since we synchronize all access to the values.
    F: FutureResult<T, LiftArgsError>,
    F: UniffiCompatibleFuture<Result<T, LiftArgsError>> + 'static,
    // T is the output of the Future.  It needs to implement [LowerReturn].  Also it must be Send +
    // 'static for the same reason as F.
    T: LowerReturn<UT> + Send + 'static,