`:request_timeout` with HTTP/2

[danschultzer]

I'm getting :request_timeout in HTTP/2 but shouldn't it just be :timeout so it conforms to HTTP/1 connections?

The problem is that Req just looks for :timeout for transient errors. This could be handled in Req, but I think it makes more sense to conform these so it just has the same :timeout reason. Is there a good reason to use :request_timeout?

I switched from HTTP/1 to HTTP/2 and things started failing due to timeouts not being caught.

[sneako]

Great catch, thank you! I think I prefer :request_timeout, but I assume there is much more HTTP1 usage than H2, so it would probably be less disruptive to change H2 to match H1.

I'm happy to accept a PR making them both use :timeout.

[danschultzer]

I've pushed #319, but I think there's something about how errors are dealt with in Finch. I think it'll be better if we encapsulate them in some generic errors. The timeout error could really be Finch.TimeoutError that can contain reason that is the inner error, e.g. Mint.TransportError or whichever. That will be massive breaking change though, but I think it'll be cleaner to have common normalized errors across the board as in most cases you only really care that e.g. a timeout occurred not really the specifics of the timeout (e.g. it was HTTP/2 request timeout).

[VasiliyS]

I've also faced similar issues recently when trying to use HTTP2 with Req. When this gets merged, will this require action from the Req maintainers?

I'm asking as I haven't seen any HTTP2 related issues in their repo. I'm happy to file one and reference this issue, if that helps with a faster resolution, just didn't want to create unnecessary noise.