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.

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

feat(transport): add unix socket support in server #861

Merged

Conversation

agreen17
Copy link
Contributor

@agreen17 agreen17 commented Dec 8, 2021

Addresses #856

Adds tonic::transport::server::UdsConnectInfo to facilitate consumers using a unix domain socket on the server side. Updates the existing uds example to use this UdsConnectInfo and tokio's UnixListenerStream as the latter cuts down on consumer boilerplate. Adds an integration test for UdsConnectInfo.

Motivation

Currently using a unix socket requires consumers to provide an implementation of the tonic::transport::server::Connected trait (UdsConnectInfo).

When I started using tonic, I copy+pasted this stuff from the uds example 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 a tonic server.

This PR aims to allow consumers to more easily use a tokio::net::UnixListener with a tonic server.

Solution

The UdsConnectInfo implementation here is pulled directly from the existing uds example. I pulled UnixListenerStream into the uds example to remove some boilerplate code there.

Comment on lines 89 to 92
let unix_socket_path = "/tmp/tonic/uds-integration-test";
tokio::fs::create_dir_all(Path::new(unix_socket_path).parent().unwrap())
.await
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/// See [`Connected`] for more details.
///
/// [ext]: crate::Request::extensions
#[cfg(unix)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just move this to a unix.rs file and cfg that whole file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍🏽

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments but I like this idea, we should have probably added this a while ago.

@agreen17
Copy link
Contributor Author

agreen17 commented Jan 13, 2022

Left a few comments but I like this idea, we should have probably added this a while ago.

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 agreen17 force-pushed the add-unix-socket-support-in-server branch from 4933210 to 9aa29e6 Compare January 13, 2022 15:21
@agreen17
Copy link
Contributor Author

Oops, looks like I missed a #[cfg(unix)] in the UDS example and that broke non-Unix builds. Just pushed a fixup to address that

@agreen17 agreen17 force-pushed the add-unix-socket-support-in-server branch from c195e2a to 1956549 Compare January 21, 2022 16:22
@ShaneEverittM
Copy link

Is this gonna land in the next release?

@ShaneEverittM
Copy link

Also, does this support abstract sockets?

Comment on lines 35 to 38
// 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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense to have these as doc items and not have asserts in an example, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that works for me then we can merge! Thank you for the patience on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LucioFranco For sure! Thanks for taking the time to review.

Fixup commits have been squashed - hopefully we're good to merge now 👀

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM just would like to see the asserts removed.

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 <agreen@starry.com>
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 <agreen@starry.com>
Refs: hyperium#856

Signed-off-by: Anthony Green <agreen@starry.com>
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@LucioFranco LucioFranco changed the title feat(tonic): add unix socket support in server feat(transport): add unix socket support in server Feb 14, 2022
@LucioFranco LucioFranco merged commit dee2ab5 into hyperium:master Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants