Any interest in implementing tonic::transport::server::Connected for tokio::net::UnixStream?

[agreen17]

Feature Request

I started using tonic recently and it has been awesome to work with so far! I saw an opportunity for a small contribution that I believe would make tonic a little easier to use so I wanted to see if there was interest in accepting such a feature - implementing tonic::transport::server::Connected for tokio::net::UnixStream to allow client code to use a UnixStream on the server side more easily.

Crates

tonic

Motivation

At first glance I thought there was no Unix socket support for tonic clients/servers, but I stumbled upon the uds client/server example soon thereafter and I was able to get my server listening on a unix socket easily.

That said, I think it would be nice if consumers could use a tokio::net::UnixStream on the server side without needing to provide the following (which are currently required):

• impl tonic::transport::server::Connected for tokio::net::UnixStream
• newtype wrapper (or similar) around tokio::net::UnixStream, implement AsyncRead/AsyncWrite (only needed since client code can't implement an external trait for an external type)

Proposal

I've looked around the repo a bit and I think the main piece of getting this working would be something like this:

// in `transport/server/conn.rs` - copied from the current UDS example

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[derive(Clone, Debug)]
pub struct UdsConnectInfo {
    /// Peer address. This will be "unnamed" for client unix sockets.
    pub peer_addr: Option<Arc<tokio::net::unix::SocketAddr>>,
    /// Process credentials for the unix socket.
    pub peer_cred: Option<tokio::net::unix::UCred>,
}

#[cfg(unix)]
impl Connected for tokio::net::UnixStream {
    type ConnectInfo = UdsConnectInfo;

    fn connect_info(&self) -> Self::ConnectInfo {
        UdsConnectInfo {
            peer_addr: self.peer_addr().ok().map(Arc::new),
            peer_cred: self.peer_cred().ok(),
        }
    }
}

To me, the win here is that since tonic itself would be implementing Connected, client code would no longer have to define a newtype wrapper around UnixStream and hand-implement Connected/AsyncRead/AsyncWrite on it - tokio-streams UnixListenerStream would then be usable out the box at this point:

let greeter = MyGreeter::default();

let uds = tokio::net::UnixListener::bind(path)?;
let uds_stream = tokio_stream::wrappers::UnixListenerStream::new(uds);

Server::builder()
    .add_service(GreeterServer::new(greeter))
    .serve_with_incoming(uds_stream)
    .await?;

This could possibly require some other work in server/mod.rs but I'm unsure about this part...just wanted to check in on interest level before digging in too far! Thanks for looking!

Alternatives

Current solution is to copy the approach from the uds/server.rs example, which works right now as-is.

[LucioFranco]

Yeah, I think this makes sense, I would totally accept a PR for this.

[agreen17]

Yeah, I think this makes sense, I would totally accept a PR for this.

Awesome. I'll submit a PR for this in the next few days.

[agreen17]

@LucioFranco (or someone else - not really sure who to ask 😁 ) could you check out this PR I put out for the fix and allow me to run workflows? Looks like I need a maintainer to approve running CI

[LucioFranco]

Yes just took a look, sorry for the delay. Holidays are crazy :D

[agreen17]

@LucioFranco Haha no worries, thanks for taking a look 👍🏽

[agreen17]

Closing this issue as the the relevant PR has been merged 🎉