Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default timeouts for the HTTP clients #44

Closed
zimbatm opened this issue Jul 18, 2013 · 2 comments
Closed

Default timeouts for the HTTP clients #44

zimbatm opened this issue Jul 18, 2013 · 2 comments

Comments

@zimbatm
Copy link
Contributor

zimbatm commented Jul 18, 2013

Right now it's not possible to configure the timeout defaults of the Net::HTTP clients (and possibly EM::HTTPClient).

Here are the defaults on Net::HTTP:

keep_alive_timeout = 2
# Depends on system settings. Usually 240 seconds on Linux.
open_timeout = nil
read_timeout = 60
continue_timeout = nil
ssl_timeout = nil

In the context of a webapp you usually want to lower these numbers to stay below a ~5 second threshold and then return an error message to the user if the connection or response didn't happen. The webapp usually has fast connectivity so we don't need to be so forgiving during the connection. Issues are more related to downtime or equipment failure and giving it more time won't generally help. Heroku also disconnects the customer from the webapp after 30 seconds so in that case you also want to send a meaningful reply to the customer instead of the default heroku 500 message.

I don't have the time right now to propose a patch so until them I'm going to use the following monkey-patch. If you add a setting to these defaults make sure to catch the "Net::OpenTimeout" exception in https://github.com/pusher/pusher-gem/blob/master/lib/pusher/request.rb#L40

Cheers !

require 'net/http'
module Net
  class HTTP < Protocol
    alias default_timeout_initializer initialize
    def initialize(address, port = nil)
      default_timeout_initializer(address, port)
      #@keep_alive_timeout = 2
      @open_timeout = 2
      @read_timeout = 5
      @continue_timeout = 5
      @ssl_timeout = 2
    end
  end
end
@mloughran
Copy link
Contributor

Thanks for digging into this Jonas - you're completely right the timeout should be configurable. We're planning to switch from Net::HTTP for a variety of reasons, but I'll keep this in mind.

@mloughran
Copy link
Contributor

@zimbatm would be great if you could take the new version for a spin - you can easily set all the timeouts via Pusher.timeout = 5, or set the individual timeouts if you like (see client.rb). The keepalive change should make a big difference too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants