Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backend plugin api is not FFI safe #404

Open
imtsuki opened this issue Apr 18, 2024 · 2 comments
Open

Backend plugin api is not FFI safe #404

imtsuki opened this issue Apr 18, 2024 · 2 comments

Comments

@imtsuki
Copy link
Member

imtsuki commented Apr 18, 2024 •
edited
Loading

Currently Rust compiler does not provide any guarantee for ABI stability across FFI boundaries, so the calling convention, trait object layout, etc. might be different across compiler versions or even different builds. Some issues in current backend plugin implementation include

  1. register_plugin is not extern "C";
  2. register_plugin returns a trait object, which is not FFI-safe;
  3. async_trait desugars to something like fn () -> Box<dyn Future>, which also returns a trait object;
  4. parameters like HeadRequest are not repr(C).

client/dragonfly-client-backend/examples/plugin/src/lib.rs

Lines 31 to 51 in b2ff71c

#[tonic::async_trait]
impl Backend for Hdfs {
// head is an async function that takes a HeadRequest and returns a HeadResponse.
async fn head(&self, request: HeadRequest) -> Result<HeadResponse> {
println!("HDFS head url: {}", request.url);
Err(Error::Unimplemented)
}
// get is an async function that takes a GetRequest and returns a GetResponse.
async fn get(&self, request: GetRequest) -> Result<GetResponse<Body>> {
println!("HDFS get url: {}", request.url);
Err(Error::Unimplemented)
}
}
// register_plugin is a function that returns a Box<dyn Backend + Send + Sync>.
// This function is used to register the HDFS plugin to the Backend.
#[no_mangle]
pub fn register_plugin() -> Box<dyn Backend + Send + Sync> {
Box::new(Hdfs::new())
}

I suggest to use some libraries like https://docs.rs/async-ffi/latest/async_ffi/ and https://docs.rs/abi_stable/latest/abi_stable/; or if we want to support plugins written in other languages, we might need to re-consider the interface design, particularly regarding how should we handle async operations like Futures.
@gaius-qi
Copy link
Member

@imtsuki No need to support plugin written in other languages. It is necessary to provide a build image of the plugin to make a consistent environment, refer to https://d7y.io/docs/next/contribute/development-guide/plugin-builder/.

@xujihui1985
Copy link
Contributor

@gaius-qi I think what @imtsuki mean is if we want to load .so dynamically as what the example doing now, we need to take ffi safety into consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@xujihui1985 @imtsuki @gaius-qi