Make serve generic over the listener and IO types
#2941
Conversation
jplatte
force-pushed
the
jplatte/generic-serve
branch
2 times, most recently
from
53c870c
to
72871dc
Compare
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
jplatte
force-pushed
the
jplatte/generic-serve
branch
from
72871dc
to
26a37a4
Compare
| async fn accept<L>(listener: &mut L) -> Option<(L::Io, L::Addr)> | ||
| where | ||
| L: Listener, | ||
| { |
There was a problem hiding this comment.
How much TCP-specific is this method? I'm especially concerned with the logic regarding sleeps. Different implementations could use different set of ignorable errors than what we have in is_connection_error and then this could add needless sleeps and error logs.
I think this logic should be moved up to the impl Listener block and not be used for all IOs. Or again defer to a more specific axum-provided Listener or mapping function (this is similar to the other comment).
| let tcp_stream: &tokio::net::TcpStream = <dyn std::any::Any>::downcast_ref(&io) | ||
| .expect("internal error: tcp_nodelay used with the wrong type of listener"); |
There was a problem hiding this comment.
I'd rather we avoid special-casing like this.
I assume that the end game would be either a) to have these configurations on a specific Listener (like axum::TcpListener) which would accept a connection and set this and possibly other configurations or b) have something like ListenerExt::map_io -> impl Listener which could prepare each TcpStream (or any other Listener::Io) with additional connection options.
Both of these are also possible from user code so I think even if we didn't want to create either right now, we could just drop the first class nodelay support.
How do you see the end goal regarding this?
| type Addr = std::net::SocketAddr; | ||
|
|
||
| #[inline] | ||
| async fn accept(&mut self) -> io::Result<(Self::Io, Self::Addr)> { |
There was a problem hiding this comment.
Currently, we ignore these errors. Or more precisely, we ignore "connection errors" and log and sleep on all other errors.
I think we could just have a return type of (Self::Io, Self::Addr) here and let the underlying method do whatever it wants to handle transient accept errors, log them, sleep, panic, or anything else.
Or we could let it return a Result but then I think those errors should be fatal and we'd just propagate them. And in that case I'd also consider whether to have an associated type for the error type instead of forcing std::io::Error.
Supersedes #2479.