async-ffi icon indicating copy to clipboard operation
async-ffi copied to clipboard

Extend to `FfiStream<T>`?

Open cutsoy opened this issue 2 years ago • 3 comments

Great library!

I'm using this for FFI-safe Stream<T>s and it was very easy to build based on your work. FFI streams are very similar to futures:

#[repr(C)]
pub struct LocalBorrowingFfiStream<'a, T> {
    str_ptr: *mut (),
    poll_fn: unsafe extern "C" fn(
        str_ptr: *mut (),
        context_ptr: *mut FfiContext,
    ) -> FfiPoll<FfiOption<T>>,
    drop_fn: unsafe extern "C" fn(*mut ()),
    _marker: PhantomData<&'a ()>,
}

I could file a PR with the implementation if you think it's in scope for this library (maybe behind a feature flag)?

cutsoy avatar Feb 18 '23 09:02 cutsoy

I'm assuming that you are talking about the trait Stream from futures crate. Since tokio switches to futures-core, I think that's the only popular API for Stream now.

FFI streams are very similar to futures

They are exactly the same ABI to me, except that it allows extra polls after one returning Ready. Note that Option is already FFI-safe for some specific types like references and Boxes. We can just makes FfiStream<T> FFI-safe as long as Option<T> is so, thus it's users' duty to guarantee it.

Then we can have:

#[cfg(feature = "futures")]
pub use crate::futures::{...};
#[cfg(feature = "futures")]
mod futures {
    #[repr(transparent)]
    pub struct BorrowingFfiStream<'a, T>(BorrowingFfiFuture<'a, Option<T>>);
    impl BorrowingFfiStream<'a, T> {
        pub fn new<S: Stream>(stream: S) { ... }
    }
    impl Stream for BorrowingFfiStream<'a, T> { ... }

    // pub struct LocalBorrowingFfiStream
    // pub type FfiStream
    // pub type LocalFfiStream

    pub trait StreamExt: Stream {
        fn into_ffi<'a>(self) -> BorrowingFfiStream<'a, Self::Item> where Self: 'a;
        // fn into_local_ffi
    }
    impl Stream for BorrowingFfiStream<'a, T> { ... }
}

BTW: I really don't like the long Borrowing prefix, but I don't know how often it's used relative to 'static one.

oxalica avatar Feb 18 '23 23:02 oxalica

Oh that's really neat! I didn't even realize you could turn it into a (kind of misbehaving) future.

I can probably turn this into a PR if you want!

3 remarks:

  1. The documentation doesn't unequivocally state that Option<T> is ABI-safe, and in any case this optimization only works if T is a non-nullable pointer (which is usually not the case).
  2. Regarding Borrowing, I guess it would also be fine to make it the default and let users explicitly specify FfiStream<'static, T> if it's non-borrowing.
  3. Indeed, I don't see a particular use case for crossing the FFI boundary with a non-'static lifetime. That's inherently unsafe so the caller might as well transmute it into 'static if necessary.

cutsoy avatar Feb 19 '23 14:02 cutsoy

The documentation doesn't unequivocally state that Option<T> is ABI-safe, and in any case this optimization only works if T is a non-nullable pointer (which is usually not the case).

Yes. But if you want to pass a Option<T>, most common ways are

  1. Option<Box<RustStruct>>
  2. ReprCEnum { None, Some(ReprCStruct) }

(1) is much easier and flexible to me.

Regarding Borrowing, I guess it would also be fine to make it the default and let users explicitly specify FfiStream<'static, T> if it's non-borrowing. Indeed, I don't see a particular use case for crossing the FFI boundary with a non-'static lifetime. That's inherently unsafe so the caller might as well transmute it into 'static if necessary.

I agree that we could enforce to explicit write Ffi*<'static, T> . But the lifetime here behaves as the marker for guarantees, which is important no matter the code language of the other side of FFI.

fn do_something<'a>(ctx: &'a SomeContext) -> BorrowingFfiFuture<'a, ()>;

It reads, the argument pointer must not be dropped before the returned Future (its data pointers) completes. The lifetime also makes things easier if two sides are both Rust.

oxalica avatar Feb 20 '23 04:02 oxalica