cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.3k stars 1.1k forks source link

Not possible to implement `pingora::services::Service` due to `Fds` not being available. #145

Closed halzy closed 3 months ago

halzy commented 3 months ago

Describe the bug

When trying to implement pingora::services::Service the Fds struct is not public because the transfer_fd module is private: error[E0603]: module `transfer_fd` is private

Pingora info

Pingora version: 0.1.0 Rust version: cargo 1.76.0 (c84b36747 2024-01-18) Operating system version: OSX 14.2.1 (23C71)

Steps to reproduce

When trying to implement the pingora::services::Service trait it is not possible due to the 2nd argument of start_service - fds type not being pub:

struct MyService {}

#[async_trait]
impl pingora::services::Service for MyService {
    async fn start_service(
        &mut self,
        fds: Option<Arc<tokio::sync::Mutex<pingora::server::transfer_fd::Fds>>>,
        mut shutdown: pingora::server::ShutdownWatch,
    ) {
        todo!()
    }

    fn name(&self) -> &str {
        todo!()
    }
}

Expected results

The Fds or ListenFds type is public.

In pingora-core/src/server/mod.rs ListenFds is pub(crate): 52:pub(crate) type ListenFds = Arc<Mutex<Fds>>;

in pingora-core/src/server/mod.rs the transfer_fd mod is pub(crate): 19:pub(crate) mod transfer_fd;

Observed results

What actually happened?

error[E0603]: module `transfer_fd` is private

halzy commented 3 months ago

If one tries to copy the Fds struct up into their app to implement it the error is:

error[E0053]: method `start_service` has an incompatible type for trait

The struct copied is:

pub struct Fds {
    map: HashMap<String, std::os::fd::RawFd>,
}
bsodmike commented 3 months ago

Thanks @halzy I've patched this in #149

halzy commented 3 months ago

Thanks @halzy I've patched this in #149

I see. I'm not certain that the other public functions in that module are useful in the public interface as they are part of the machinery for transfering the sockets between pingoras. 🤷 We'll see what the folks at Cloudflare want to do.

bsodmike commented 3 months ago

Do you have any sample code that actually uses the contents of Option<Arc<tokio::sync::Mutex<pingora::server::transfer_fd::Fds>>>. That would help me provide a better answer.

halzy commented 3 months ago

Nothing that I can share at the moment. I posted the patch that solved my issue in #146.