Introduce :request_timeout option

[jswanner]

Intended to timeout a request with a chunked response after the total elapsed time has surpassed the given value. See #243.

[jswanner]

The test suite currently fails with this change with the following:

  1) test request/3 returns error when request times out for chunked response (FinchTest)
     test/finch_test.exs:503
     ** (CaseClauseError) no case clause matching: {:data, #Reference<0.1536038530.2835349507.48741>, "chunk-data"}
     code: |> Finch.request(finch_name, request_timeout: timeout * 10)
     stacktrace:
       (finch 0.16.0) lib/finch/http1/conn.ex:285: Finch.Conn.receive_response/9
       (finch 0.16.0) lib/finch/http1/conn.ex:122: Finch.Conn.request/7
       (finch 0.16.0) lib/finch/http1/pool.ex:47: anonymous fn/9 in Finch.HTTP1.Pool.request/5
       (nimble_pool 1.0.0) lib/nimble_pool.ex:349: NimblePool.checkout!/4
       (finch 0.16.0) lib/finch/http1/pool.ex:39: Finch.HTTP1.Pool.request/5
       (finch 0.16.0) lib/finch.ex:409: anonymous fn/4 in Finch.request/3
       (telemetry 1.0.0) /path/to/finch/deps/telemetry/src/telemetry.erl:293: :telemetry.span/3
       test/finch_test.exs:527: (test)

This case statement doesn't match because the Reference in {:data, #Reference<>, "chunk-data"} doesn't match ref:

https://github.com/sneako/finch/blob/d3c406bc29209cb9468c3a742d394960e11fedf1/lib/finch/http1/conn.ex#L285-L332

[jswanner]

@sneako, I thought of a way to implement this that does work, but it does require spawning another process. I understand if that's an undesirable implementation detail. If you're happy with the changes, I'm happy to squash the commits.

[sneako]

Thank you @jswanner but yes, spawning a process here is definitely not something we can add to the library, it will severely impact performance.

In order to have a very accurate and strict timeout, an additional process is probably necessary, but I was thinking in this case we can have a timeout with much looser guarantees that will still provide some value.

Basically this new timeout would be a value which will be decremented by the duration of each individual recv call. This should be "good enough" unless our calls to mint block indefinitely (shouldn't happen unless :infinity is passed in as the recv timeout) or if the Finch user is performing a lot of slow work in their stream function.

Does that make sense to you?

[wojtekmach]

Sorry if this is silly but I always wondered if instead of doing the work in a separate process we could spawn a new process that would sleep for timeout and kill the parent? Something like this:

parent = self()

pid =
  spawn(fn ->
    Process.sleep(1000)
    Process.exit(parent, :timeout)
  end)

# do work...

Process.exit(pid, :normal)

Again this might be a dumb idea and so yeah I believe the best effort approach is warranted here. :)

@jswanner btw, I think I briefly saw you on the conference? If so I wish we had a chance to talk more about this!

[jswanner]

Basically this new timeout would be a value which will be decremented by the duration of each individual recv call. This should be "good enough" unless our calls to mint block indefinitely (shouldn't happen unless :infinity is passed in as the recv timeout) or if the Finch user is performing a lot of slow work in their stream function.

Does that make sense to you?

@sneako, it does make sense, and that's basically what I tried in the first commit of this PR. Unfortunately some weird stuff was happening in the test that I couldn't explain, it seems an additional request was queued and pipelined then data was received for a different request ref.

@wojtekmach that's an interesting idea! I might have to play around with that just for my own curiosity. And yes, we did briefly chat at the very end of the conference.

[jswanner]

@sneako, I've removed the spawning of a new process, and figured out what was going on. Let me know what you think.

[sneako]

Thank you so much @jswanner and please accept my apologies for the delays, I've had some major life events happen recently but am finally feeling ready to get back on the horse. I would like to add a bit to the docs for the new option, but otherwise this looks great to me

[jswanner]

@sneako, hopefully they were happy major life events and not sad ones.

Great points on the option's documentation. Let me know what you think of the changes I made.