Add :timeout and :deadline options

[wojtekmach]

The idea is for something like this:

Req.get!(url, timeout: 5000)

to be as close as possible to:

Task.async(fn ->
  Req.get!(url)
end)
|> Task.await(5000)

I think under the hood we'd do some bookkeeping around Finch connect timeout and request_timeout (I believe it's a mix of pool_timeout and receive_timeout). We need to make sure it works on HTTP/2 too.

There's some ambiguity around retries, whether :timeout should be per retryable request or for the whole pipeline. Per snippet above I believe it should the latter, one timeout for entire pipeline. I think for :deadline it will pretty obviously be the entire pipeline too so there's an argument that :timeout and :deadline should be equivalent, just two different ways to encode the same things.

WDYT @whatyouhide?

[whatyouhide]

What would :deadline be, a time in the future that the request needs to be done by?

As for retries, it's hard. Do you want to yell at people if they use a total timeout that is potentially incompatible with the retries they've specified, for example?

[wojtekmach]

RE :deadline, yeah, exactly. One example is :deadline option in DBConnection.execute/4. There is a tiny note about it here: elixir-ecto/db_connection#108, but more broadly I've heard people wanting to pass down such deadline throughout the stack (think Golang context thing) and in some cases don't even make certain calls if they are past deadline.

RE retry, good question. I'd keep it orthogonal and dumb, i.e. once we make retries opt-in (#383) I'd like to have a shorthand like retry: true and then this timeout would always be opt-in so if the timeout is less than theoretical upper retry timeout, that's OK, the smaller value wins. I'd like to think it's reasonable enough, WDYT?

[whatyouhide]

I'd keep it orthogonal and dumb

Yeah it probably is reasonable enough. 🙃 Sounds good.

[wojtekmach]

Come to think of it, I suppose we'd need to add a caveat (similar to the one from Finch :request_timeout) that this would be a best effort timeout/deadline and if someone needs a guarantee they should use async/await. Best effort as in we'd pass timeout/deadline to calls that support them (I/O) but otherwise I don't think we want to, for example, check timeout/deadline before running each step. Or maybe we should just wrap it in async/await ourselves and document we're doing so and thus there's extra data copying, though I'd really rather avoid that.

[whatyouhide]

Yeah good point, the steps can add to the total timeout. Maybe I’m thinking this: we can add a timeout to Req that translates to the actualy network timeout (so the same as the Finch one). Then, we don't add :timeout/:deadline and tell people to async/await (or we do async/await in Req as you said). Async/await is cheap and we could figure out a way to only use it for the timeout, like sending the response directly to the caller rather than the async/await proc.

[wojtekmach]

Thanks, sending the response back to the caller bypassing async process so to speak is interesting, I can look into that. But yeah I think we either do a :timeout using async/await or we don't and just call it :network_timeout/:request_timeout/etc. In other words we don't do a :timeout that does not have a strong guarantee. The former, timeout with async/await, is definitely more compelling if we can swing it in efficient way, have errors with stack traces etc.