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

Fix reconnect on keep alive timeout when allowing all connections. #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dylanahsmith
Copy link

Problem

Ruby 2.0 added a keep_alive_timeout variable to Net::HTTP for persistent. If keep_alive_timeout seconds has passed between requests, then Net::HTTP will close the socket and call connect. However, fakeweb replaces the connect method to have it do nothing (delaying it until request is called), so the socket is closed without reconnecting on a keep_alive_timeout.

The following script will result in a closed stream (IOError) exception being raised, but will run without exceptions after removing require 'fakeweb'.

require 'net/http'
require 'fakeweb'

uri = URI("http://www.google.com/")
req = Net::HTTP::Get.new(uri)

Net::HTTP.new(uri.host, uri.port).start do |http|
  http.request(req)
  sleep 2  # default keep_alive_timeout
  http.request(req)
end

fakeweb sets FakeWeb.allow_net_connect = true on initialization. In this state, we should avoid changing the behaviour of Net::HTTP so that it works in cases that fakeweb wasn't designed for.

Solution

Add FakeWeb.allow_all_connections? which returns true if fakeweb isn't intercepting any any requests (i.e. FakeWeb.allow_net_connect = true). In this state, the Net::HTTP monkey patch returns the behaviour of the connect and request method by just calling the real methods.

Remaining Problem

fakeweb will still have this problem for persistent http connections when passthrough uri's are registered, but I didn't want to complicate this pull request by trying to solve this use case. I wanted to start by making sure fakeweb isn't disruptive when it is required but isn't being used, or when it is disabled for certain tests.

Update: created pull #44 to address this problem.

@salimane
Copy link

Any update on this ?

@dcosson
Copy link

dcosson commented Jun 1, 2016

Any update on this?

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

Successfully merging this pull request may close these issues.

3 participants