Add generic trait to combine UnixListener and TcpListener #4244
Conversation
|
@Darksonn you suggested such a trait in tokio-util. Is this what you had in mind? |
| /// Accepts a new incoming connection from this listener. | ||
| fn accept<'a>( | ||
| &'a self, | ||
| ) -> Pin< | ||
| Box< | ||
| dyn Future< | ||
| Output = Result<(Box<dyn AsyncReadWrite + Send + Unpin + 'static>, ListenAddr)>, | ||
| > + Send | ||
| + 'a, | ||
| >, | ||
| >; |
There was a problem hiding this comment.
If we want to add a trait, I think it should use a poll fn rather than a boxed future.
| pub trait AsyncReadWrite: tokio::io::AsyncRead + tokio::io::AsyncWrite { | ||
| /// Sets the value of the `TCP_NODELAY` option on this socket if this is a TCP socket. | ||
| fn set_nodelay(&self, nodelay: bool) -> Result<()>; | ||
| } |
There was a problem hiding this comment.
The name of this trait doesn't make sense. It's not a trait for combining AsyncRead and AsyncWrite - its a trait for adding an set_nodelay method to a type.
There was a problem hiding this comment.
This should probably be called...SetNodelay, and not require AsyncRead and AsyncWrite...
There was a problem hiding this comment.
It's used for boxing.
Box<dyn AsyncReadWrite + Send + Unpin + 'static>
You can't add 2 traits in a Box dyn like that. You have to create a third one that inherit the 2 others.
set_nodelay here is detail in the trait. It's there because I needed it from TcpListener but other functions could be there too
There was a problem hiding this comment.
You can't add 2 traits in a Box dyn like that. You have to create a third one that inherit the 2 others.
Right...I think that the boxed trait object should probably be replaced with an associated type like
type Io = tokio::io::AsyncRead + tokio::io::AsyncWrite;as Alice mentioned here #4244 (comment). That way, user code could define a SetNodelay trait, implement it for TcpStream and UnixStream, and have trait bounds like
async fn do_something_with_a_listener<L>(listener: L)
where
L::Io: SetNodelay,
{
// ...
}This way, if other users need to expose other functionality on sockets, they can add similar traits and still use the Listener trait, rather than special-casing set_nodelay just because your application needs to disable Nagle's algorithm.
There was a problem hiding this comment.
Yes, this should be an associated type, and the set_nodelay method should honestly just get taken out. It's too specific to your particular use-case.
| ) -> Pin< | ||
| Box< | ||
| dyn Future< | ||
| Output = Result<(Box<dyn AsyncReadWrite + Send + Unpin + 'static>, ListenAddr)>, |
There was a problem hiding this comment.
Rather than returning a boxed IO resource, I would prefer to use an associated type.
(And if we did want to return a boxed IO resource, it should have been a Pin<Box<...>> rather than an Box<... + Unpin>.)
| >; | ||
|
|
||
| /// Returns the local address that this listener is bound to. | ||
| fn local_addr(&self) -> Result<ListenAddr>; |
There was a problem hiding this comment.
Also seems like this should be an associated type rather than return an enum with the two types hard-coded. Otherwise the trait is designed to only allow TcpListener and UnixListener, and people would not be able to implement it for their own listeners, e.g. maybe there's a listener for TLS streams.
There was a problem hiding this comment.
My idea was to make it only for TcpListener and UnixListener 😁 it didn't even crossed my mind to support other things.
I don't mind working on making this more generic but I need to update the title of this issue then.
|
I'm closing this because the implementation is going in the wrong direction and there hasn't been any activity for a while. Feel free to reopen or open a new issue if you want to continue working on this. |
|
I'm back on this! 😁 Unfortunately I can't reopen the ticket on my side. I already pushed one new commit that replaces the box with the associated type but it's not visible here for some reason. Should I open a new PR? cc @Darksonn |
|
Yeah, just open a new PR. |
Motivation
I would like to have a generic type for creating a socket from TCP or from a Unix socket so I don't need to duplicate code.
Solution
accept()method and alocal_addr()method to reflect the methods from TcpListener and UnixListener.set_nodelay()to reflect the methodset_nodelay()of TcpStream which I needed.Problem encountered
There is a module "udp" at root of tokio-util. It should probably moved to the new module "net" I created. One alternative would be to rename the "unix" module I created to something else.