faraday: Distinguish TimeoutErrors for open and read timeouts

In faraday/adapter/rack.rb, TimeoutError is raised for both open and read timeouts:

timeout  = env[:request][:timeout] || env[:request][:open_timeout]
response = if timeout
  Timer.timeout(timeout, Faraday::Error::TimeoutError) { execute_request(env, rack_env) }
else ... end

According to https://stackoverflow.com/questions/10322283/what-is-timeout-and-open-timeout-in-faraday, open_timeout is for the tcp connection and timeout is for the response read.

It would be nice to have separate exception types for these timeouts. Then we could determine whether or not to retry the request. Does adding something like Faraday::Error::OpenTimeoutError and Faraday::Error::ResponseTimeoutError and using those here make sense?

About this issue

Original URL
State: open
Created 7 years ago
Reactions: 1
Comments: 32 (25 by maintainers)

Commits related to this issue

Handle net http open timeout error. — committed to lostisland/faraday by mistersourcerer 7 years ago
Upgrade json, restforce, and webmock to latest versions. The versions of each of these had fallen a reasonable way behind mainline, which could cause issues when trying to depend on this code in more... — committed to adam-harwood/salesforce_bulk_query by deleted user 3 years ago

Most upvoted comments

Hi @coberlin I believe this might be a nice addition, I’m just scared about backwards compatibility. However, a possible solution for this might be to have OpenTimeoutError and ResponseTimeoutError to inherit from TimeoutError, so that existing rescues will keep working as expected. It’s definitely worth some testing 😃

iMacTia on Aug 10, 2017

We’re running into OpenTimeout errors with an API endpoint occasionally that need to be retried; it seems from your logic above that open timeouts are safe to retry (more safe than a read timeout). We also had assumed that with the retry middleware, both open and read timeouts would be retried; the documentation reads, “By default, it retries 2 times and handles only timeout exceptions.” The default exceptions handled are Errno::ETIMEDOUT, Timeout::Error, Error::TimeoutError, and Net::OpenTimeout is a subclass of Timeout::Error; it wasn’t particularly clear that Faraday was treating them differently. So perhaps the documentation should be updated? In any case, yes, we configured the middleware; I’m just wondering if the default makes sense.

mjhoy on Aug 22, 2018

Throwing a use-case into the ring:

At work we’ve been suffering from some Open Timeouts due to Nginx + Kubernetes failing to route to hanging pods (or something). Anyway, NetHTTP used to throw OpenTimeout and ReadTimeout errors, and that was really handy for us debugging which was which.

Now we’ve switched to Typhoeus we sadly have all timeouts munged together, and it’s tough for us to tell if our work on the nginx + kuber problems have been improved, or if we’re just now successfully making more requests to an increasingly struggling system. Either way the number of timeouts are about the same, and without getting them separated we’re kinda stuck guessing.

I don’t think just adding Faraday::OpenTimeoutError is enough, we should have Faraday::OpenTimeoutError and Faraday::ReadTimeoutError extending from Faraday::TimeoutError IMO.

philsturgeon on Nov 14, 2017