add Server::from_tcp() #1624
add Server::from_tcp() #1624
Conversation
|
Thanks for starting this!
This is likely the reason CI is failing, it's not compatible with the minimum supported Rust version. I'd change to returning
These enable
Last time I tried out rustfmt (admittedly a while ago), it broke some things. I've just pushed a
This is probably fine! Or... I mean, maybe it could be converted to the internal |
yoshuawuyts
force-pushed
the
from_tcp
branch
6 times, most recently
from
6422474
to
10628d4
Compare
|
Okay, updated the errors & rustfmt!
Hmm, I don't have enought background knowledge to comment on this. The only thing I know is that it'd be slightly harder to implement - but happy to do that if you think that's the right thing to do! - would probably need a bit of guidance on it though, haha. Bar tests passing, I think this PR should be mostly done now! Are there any other things that need adding? |
src/server/mod.rs
Outdated
| @@ -96,6 +106,19 @@ impl<I> Server<I, ()> { | |||
| } | |||
| } | |||
|
|
|||
| impl Server<tokio_tcp::Incoming, ()> { | |||
| /// Create a new instance from a `std::net::TcpListener` instance. | |||
| #[cfg(feature = "runtime")] | |||
There was a problem hiding this comment.
It seems CI noticed that this cfg should go on the impl instead.
There was a problem hiding this comment.
Actually, since this should change to convert into an AddrIncoming, this whole impl can just be merge with the one below it...
src/server/mod.rs
Outdated
| #[cfg(feature = "runtime")] | ||
| pub fn from_tcp( | ||
| listener: net::TcpListener, | ||
| ) -> Result<Builder<tokio_tcp::Incoming>, ::Error> { |
There was a problem hiding this comment.
I thought about it some more, and I do think it makes sense to convert into the internal AddrIncoming type instead. The changes will be easy, and then anyone using it will be able to take advantage of the default improvements like sleeping when max FDs is reached.
The changes are to add a pub(super) fn from_tcp() to AddrIncomong. The new function can be split, such that after getting the std_listener, it just calls AddrIncoming::from_tcp internally. And then this function also can just call AddrIncoming::from_tcp(listener).
There was a problem hiding this comment.
hey, I was about to send in a similar patch. I wrote hyper-tls-hack , an implementation of AddrIncoming that supports TLS using tokio-tls. It has a from_std_listener(std_listener: StdTcpListener,.... method, which I was about to split off and add to hyper's AddrIncoming. See the code here .
yoshuawuyts
force-pushed
the
from_tcp
branch
4 times, most recently
from
b155ae7
to
f6fae7f
Compare
Adds the `from_tcp()` method as per hyperium#1602. fix clippy changes
|
@seanmonstar updated the patch again with your feedback; hope this is good! ✨ |
|
Thank you! \o/ |
|
Yayay, so happy this landed! Thanks for all the help! 🎉 |
Heya 👋, this implements #1602. I'm pretty sure there's a few rough edges to this PR, but I wanted to kick things off anyway.
Questions
Error handling
The result type for
from_tcp()is now:I wasn't sure what the best way to approach the errors was; any pointers here would be fantastic!
Runtime Features
I wasn't sure what the runtime features are for (
no_stdrelated perhaps?), but figured I'd follow the prior art and add them where it seemed appropriate.Rustfmt
Rustfmt tends to run on every save; sorry if it changed things it shouldn't have. Happy to go back in and revert changes - but also happy to leave it if it doesn't matter 😁
Method name
I ended up going with
.from_tcp()because I felt it was the most descriptive, but happy to change it to any other method if people feel that's more appropriate!Tests
I only tested this on an example application to make sure it works. What would be the best way to test this?
Hope this is helpful! 🎉