Add connect, read, and write timeouts to client

[sfackler]

No description provided.

[hjr3]

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?

[seanmonstar]

hyper 0.11 didn't ship with these for a couple reasons:

• It's possible to have a rough overall timeout by using timeouts outside of hyper.
• I didn't want to pick a timer implementation that was different than the user, and then end up that multiple timer implementations were running.

Still, I realize it would be useful to have these in hyper, so let's explore!

• For anyone wanting it immediately, you can largely get most of it working externally. A "connect" timeout can be used by providing a custom 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.
• For the implementation in hyper, unless there the coarser timeouts are bad (it shouldn't be seconds off, just perhaps a few milliseconds if the timer isn't quite reached, and another loop of the 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.

[sfackler]

I wrote up a little crate handling read and write timeouts via the standard tokio-core timer: https://crates.io/crates/tokio-io-timeout

[nielsle]

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.

[hjr3]

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.

[skinowski]

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.

[hjr3]

@skinowski Definitely. Where is the advanced example? I only see the simple example.

[skinowski]

@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.

[hjr3]

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.

[hjr3]

@skinowski I also added a PR to add the general timeout to the guide hyperium/hyperium.github.io#13

[hjr3]

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.

[sfackler]

I've added implementations of read_buf and write_buf.

[hjr3]

@sfackler 👍 i will update my crate. edit: nevermind, this is a library. though i did notice i committed the Cargo.lock, which i fixed.

[softprops]

I was just going to open an issue about server side read write timeouts. Would any of the potential solutions above also cover servers?

[sfackler]

@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

[softprops]

Nice. What are the chances of something like this landing in hyper itself?

[seanmonstar]

The current situation of timers in tokio are in flux, so I have been wanting to wait for the dust there to settle.

[softprops]

Makes sense

[cetra3]

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.

[hjr3]

@cetra3 Hi, you can look at using https://crates.io/crates/hyper-timeout. I just updated it to work with hyper v0.12.

[cetra3]

Cheers @hjr3, I'm assuming this is compatible with the https connector?

[hjr3]

Yes, the example client uses the https connector.

…

On Sun, Aug 11, 2019 at 18:09 cetra3 ***@***.***> 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 <#1234?email_source=notifications&email_token=AAAIEJV6UNHFTQPAQGFUMR3QECZ55A5CNFSM4DQTAKU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4BMZ7Y#issuecomment-520277247>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAIEJT2QNCM6FHTGLXTULTQECZ55ANCNFSM4DQTAKUQ> .

[chewi]

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.

[let4be]

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

[chewi]

Yes, Hyper supports this natively now. It was added in Hyper 13 and I backported it to 12.

[EdorianDark]

It it is supported, why is this issue not closed?