Open sfackler opened 7 years ago
hyper 0.11 didn't ship with these for a couple reasons:
Still, I realize it would be useful to have these in hyper, so let's explore!
Connect
that wraps the HttpConnector
and a Timeout
. A "write" timeout (for a body) can be sort of hacked in by providing a custom Body
stream. And a "read" timeout could be used by selecting on the FutureResponse
, and/or the response body. I realize it's not super accurate, but it's there for the urgent.Core
would take a few milliseconds to get back), I'm leaning towards using tokio-core
timeouts. They're lighter weight, doesn't start a new thread, and doesn't require another dependency.I wrote up a little crate handling read and write timeouts via the standard tokio-core timer: https://crates.io/crates/tokio-io-timeout
Withoutboats has a pull request that adds middleware wrappers to tokio_service::Service. Perhaps that could be used to add logging and timing and retries to the hyper::Client
tokio-rs/tokio-service#21
This solution would be fairly generic and extendable, but it might also be slightly complicated.
Based on the discussion, I wrote a TimeoutConnector
that wraps an existing Connect
implementation here https://github.com/hyperium/hyper/compare/master...hjr3:issue-1234?expand=1. I left the code in the connect.rs
file for now, so as not to deal with module shenanigans. The TimeoutConnector
can be used with the HttpsConnector
in hyper_tls too. This may also get use closer to the suggestion from @nielsle about using Middleware. I say may because I did not like very closely that how the PR works. I still owe some tests.
The alternative is to embed the timeout in HttpConnector
too, but the wrapper seemed better/cleaner.
I will start playing around with the crate from @sfackler and see if I can get read/write timeouts setup.
Could you please update the 'advanced' client example with an overall timeout? This is the general case and if it's in the example, it'll be super helpful.
@skinowski Definitely. Where is the advanced example? I only see the simple example.
@hjr3 Here: https://hyper.rs/guides/client/advanced/
I've also figured how to do this myself:
let dur = Duration::from_millis(1000); let work = client.get(...).and_then(...)....; let tm = timer.timeout(work, dur);
But it's really easy to get caught in wrapped types of these futures: map, and_then, map_err, select2, join, etc. are not easy to follow for a beginner.
I have read/write timeouts in place. After putting those in, it seems natural to want to configure the client with a connect_timeout
instead of wrapping the Connect
implementation. I am trying to figure out how to do that.
@skinowski I also added a PR to add the general timeout to the guide https://github.com/hyperium/hyperium.github.io/pull/13
I created https://crates.io/crates/hyper-timeout to get connect, read and write timeout functionality. So far, it has been working for my use case. There may be some changes required to tokio-io-timeout, including this comment from @seanmonstar:
As a side note, looking at the tokio-io-timeout crate, the implementations of AsyncRead and AsyncWrite don't pass forward the read_buf and write_buf methods, which would mean readv and writev are no longer used for TcpStream.
I will see about getting some PRs up for tokio-io-timeout.
I've added implementations of read_buf
and write_buf
.
@sfackler 👍 ~i will update my crate.~ edit: nevermind, this is a library. though i did notice i committed the Cargo.lock
, which i fixed.
I was just going to open an issue about server side read write timeouts. Would any of the potential solutions above also cover servers?
@softprops the tokio-io-timeout crate can handle them if you manage the server accept loop manually. Example: https://github.com/sfackler/foo/blob/master/src/main.rs#L53
Nice. What are the chances of something like this landing in hyper itself?
The current situation of timers in tokio are in flux, so I have been wanting to wait for the dust there to settle.
Makes sense
Is there still plans to add read/write timeouts to client requests? I understand you can use tokio's timeout function to timeout the request overall, but this is sometimes not what you want.
Sometimes a download can be slow as heck, but it's still downloading, so as long as the time between delivery of bytes is not hit, then it's an "active" connection.
@cetra3 Hi, you can look at using https://crates.io/crates/hyper-timeout. I just updated it to work with hyper v0.12.
Cheers @hjr3, I'm assuming this is compatible with the https connector?
Yes, the example client uses the https connector.
On Sun, Aug 11, 2019 at 18:09 cetra3 notifications@github.com wrote:
Cheers @hjr3 https://github.com/hjr3, I'm assuming this is compatible with the https connector?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hyperium/hyper/issues/1234?email_source=notifications&email_token=AAAIEJV6UNHFTQPAQGFUMR3QECZ55A5CNFSM4DQTAKU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4BMZ7Y#issuecomment-520277247, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAIEJT2QNCM6FHTGLXTULTQECZ55ANCNFSM4DQTAKUQ .
We've been using hyper-timeout for a little while but it has one major drawback that only an implementation within hyper can provide. If the requested host name resolves to multiple IP addresses, hyper internally tries to connect to each one in turn. If one of them doesn't respond at all then it hangs until the OS-level timeout kicks in. This defaults to 127 seconds on Linux due to tcp_syn_retries
being 6. hyper-timeout wraps around all the connection attempts rather than each one individually so setting that timeout to something shorter means that it will give up before trying the second address. You could reduce tcp_syn_retries
but this is kernel-wide so this is far from ideal.
Is there any update on this?... fine grained control of timeout(including connecting to several IPs as mentioned by @chewi) would be really nice to have
Yes, Hyper supports this natively now. It was added in Hyper 13 and I backported it to 12.
It it is supported, why is this issue not closed?
I am going to try get a PR up for this. There are two options for timers, but I am unsure of which is best for hyper. The tokio_timer crate is more accurate but uses a dedicated thread. The Timeout in tokio-core is less accurate but does not require the extra thread.
If I was making the decision, I would create a single tokio-timer Timer (1 dedicated thread), configure it properly and then use that for any timeout needs. Does anyone else have any opinions?