feat(transport): add unix socket support in server #861
Conversation
| let unix_socket_path = "/tmp/tonic/uds-integration-test"; | ||
| tokio::fs::create_dir_all(Path::new(unix_socket_path).parent().unwrap()) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Would it make sense to use https://doc.rust-lang.org/stable/std/env/fn.temp_dir.html and not have to create a dir then we can remove the need to add the fs stuff.
There was a problem hiding this comment.
Sounds reasonable. Only small issue there is that tokio UnixListener needs the path to the unix socket to exist on instantiation, but does not clean it up when dropped. So, we need to remove the file at the end of the test or else we won't be able to run the integration tests multiple times (subsequent runs will fail with AddrInUse).
Seems like you want to remove the added tokio::fs dep, so I should be able to use std::env::temp_dir and just do a blocking rm at the end of the test std::fs::remove_file if that sounds good to you?
tonic/src/transport/server/conn.rs
Outdated
| /// See [`Connected`] for more details. | ||
| /// | ||
| /// [ext]: crate::Request::extensions | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Maybe we can just move this to a unix.rs file and cfg that whole file?
There was a problem hiding this comment.
Sounds good to me 👍🏽
Great to hear - this simplifies our server usage a bit and hopefully it will do the same for others! I've addressed your comments, let me know what you think |
agreen17
force-pushed
the
add-unix-socket-support-in-server
branch
from
4933210 to
9aa29e6
Compare
|
Oops, looks like I missed a |
agreen17
force-pushed
the
add-unix-socket-support-in-server
branch
from
c195e2a to
1956549
Compare
|
Is this gonna land in the next release? |
|
Also, does this support abstract sockets? |
examples/src/uds/server.rs
Outdated
| // Client-side unix sockets are unnamed. | ||
| assert!(conn_info.peer_addr.as_ref().unwrap().is_unnamed()); | ||
| // This should contain process credentials for the client socket. | ||
| assert!(conn_info.peer_cred.as_ref().is_some()); |
There was a problem hiding this comment.
Probably makes sense to have these as doc items and not have asserts in an example, what do you think?
There was a problem hiding this comment.
Sounds good to me 👍🏽 there's already sufficient documentation on these fields in the UdsConnectInfo struct, so I'll just go ahead and remove these assertions - the info will be printed by the statement just above anyways.
There was a problem hiding this comment.
Yup that works for me then we can merge! Thank you for the patience on this!
There was a problem hiding this comment.
@LucioFranco For sure! Thanks for taking the time to review.
Fixup commits have been squashed - hopefully we're good to merge now 👀
This impl is needed in order to use a tokio UnixStream as the `incoming` argument in methods like `tonic::transport::server::Router::serve_with_incoming_shutdown` Fixes: hyperium#856 Signed-off-by: Anthony Green <[email protected]>
tokio-stream packages a UnixListenerStream that implements futures_core::Stream. Using this cuts down on consumer boilerplate when using UnixStreams with a tonic server. Refs: hyperium#856 Signed-off-by: Anthony Green <[email protected]>
Refs: hyperium#856 Signed-off-by: Anthony Green <[email protected]>
agreen17
force-pushed
the
add-unix-socket-support-in-server
branch
from
1956549 to
5764b33
Compare
LucioFranco
changed the title
Addresses #856
Adds
tonic::transport::server::UdsConnectInfoto facilitate consumers using a unix domain socket on the server side. Updates the existingudsexample to use thisUdsConnectInfoand tokio'sUnixListenerStreamas the latter cuts down on consumer boilerplate. Adds an integration test forUdsConnectInfo.Motivation
Currently using a unix socket requires consumers to provide an implementation of the
tonic::transport::server::Connectedtrait (UdsConnectInfo).When I started using
tonic, I copy+pasted this stuff from theudsexample into my application. I bet other users have done this as well, and while this is not the end of the world, it would be nice if unix sockets worked out of the box with atonicserver.This PR aims to allow consumers to more easily use a
tokio::net::UnixListenerwith a tonic server.Solution
The
UdsConnectInfoimplementation here is pulled directly from the existingudsexample. I pulledUnixListenerStreaminto theudsexample to remove some boilerplate code there.