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:

finch/lib/finch/http1/conn.ex

Lines 285 to 332 in d3c406b

	case entry do
	{:status, ^ref, value} ->
	case fun.({:status, value}, acc) do
	{:cont, acc} ->
	receive_response(
	entries,
	acc,
	fun,
	mint,
	ref,
	receive_timeout,
	request_timeout,
	value,
	headers
	)
	{:halt, acc} ->
	{:ok, mint} = Mint.HTTP1.close(mint)
	{:ok, mint, acc, {status, headers}}
	other ->
	raise ArgumentError, "expected {:cont, acc} or {:halt, acc}, got: #{inspect(other)}"
	end
	{:headers, ^ref, value} ->
	case fun.({:headers, value}, acc) do
	{:cont, acc} ->
	receive_response(
	entries,
	acc,
	fun,
	mint,
	ref,
	receive_timeout,
	request_timeout,
	status,
	headers ++ value
	)
	{:halt, acc} ->
	{:ok, mint} = Mint.HTTP1.close(mint)
	{:ok, mint, acc, {status, headers}}
	other ->
	raise ArgumentError, "expected {:cont, acc} or {:halt, acc}, got: #{inspect(other)}"
	end
	{:data, ^ref, value} ->

[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]

I think we should mention how this is a best effort timeout and that the current implementation does not guarantee that the call will return before this amount of time passes. We should also mention that this has only been implemented for HTTP/1.

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

[sneako]

Thank you @jswanner !! ❤️