Merge pull request #5151 from chef/lcg/keepalives-again-again

HTTP 1.1 keepalives re-re-redux

6 files changed, 77 insertions, 6 deletions

diff --git a/lib/chef/cookbook/synchronizer.rb b/lib/chef/cookbook/synchronizer.rb
index 1ee30bacc7..01f6155bf3 100644
--- a/lib/chef/cookbook/synchronizer.rb
+++ b/lib/chef/cookbook/synchronizer.rb
@@ -291,7 +291,7 @@ class Chef
     end
 
     def server_api
-      Chef::ServerAPI.new(Chef::Config[:chef_server_url])
+      Thread.current[:server_api] ||= Chef::ServerAPI.new(Chef::Config[:chef_server_url], keepalives: true)
     end
 
   end
diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index 3e69f58383..ec6665f5c0 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -77,6 +77,9 @@ class Chef
 
     attr_reader :middlewares
 
+    # [Boolean] if we're doing keepalives or not
+    attr_reader :keepalives
+
     # Create a HTTP client object. The supplied +url+ is used as the base for
     # all subsequent requests. For example, when initialized with a base url
     # http://localhost:4000, a call to +get+ with 'nodes' will make an
@@ -87,6 +90,7 @@ class Chef
       @sign_on_redirect = true
       @redirects_followed = 0
       @redirect_limit = 10
+      @keepalives = options[:keepalives] || false
       @options = options
 
       @middlewares = []
@@ -228,6 +232,25 @@ class Chef
 
     def http_client(base_url = nil)
       base_url ||= url
+      if keepalives && !base_url.nil?
+        # only reuse the http_client if we want keepalives and have a base_url
+        @http_client ||= {}
+        # the per-host per-port cache here gets peristent connections correct when
+        # redirecting to different servers
+        if base_url.is_a?(String) # sigh, this kind of abuse can't happen with strongly typed languages
+          @http_client[base_url] ||= build_http_client(base_url)
+        else
+          @http_client[base_url.host] ||= {}
+          @http_client[base_url.host][base_url.port] ||= build_http_client(base_url)
+        end
+      else
+        build_http_client(base_url)
+      end
+    end
+
+    private
+
+    def build_http_client(base_url)
       if chef_zero_uri?(base_url)
         # PERFORMANCE CRITICAL: *MUST* lazy require here otherwise we load up webrick
         # via chef-zero and that hits DNS (at *require* time) which may timeout,
@@ -239,12 +262,10 @@ class Chef
 
         SocketlessChefZeroClient.new(base_url)
       else
-        BasicClient.new(base_url, :ssl_policy => Chef::HTTP::APISSLPolicy)
+        BasicClient.new(base_url, ssl_policy: Chef::HTTP::APISSLPolicy, keepalives: keepalives)
       end
     end
 
-    protected
-
     def create_url(path)
       return path if path.is_a?(URI)
       if path =~ /^(http|https|chefzero):\/\//i
diff --git a/lib/chef/http/basic_client.rb b/lib/chef/http/basic_client.rb
index 3a87fe85e4..460744ea2a 100644
--- a/lib/chef/http/basic_client.rb
+++ b/lib/chef/http/basic_client.rb
@@ -34,6 +34,7 @@ class Chef
       attr_reader :url
       attr_reader :http_client
       attr_reader :ssl_policy
+      attr_reader :keepalives
 
       # Instantiate a BasicClient.
       # === Arguments:
@@ -43,7 +44,11 @@ class Chef
       def initialize(url, opts = {})
         @url = url
         @ssl_policy = opts[:ssl_policy] || DefaultSSLPolicy
-        @http_client = build_http_client
+        @keepalives = opts[:keepalives] || false
+      end
+
+      def http_client
+        @http_client ||= build_http_client
       end
 
       def host
@@ -114,7 +119,11 @@ class Chef
 
         http_client.read_timeout = config[:rest_timeout]
         http_client.open_timeout = config[:rest_timeout]
-        http_client
+        if keepalives
+          http_client.start
+        else
+          http_client
+        end
       end
 
       def config
diff --git a/spec/unit/cookbook/synchronizer_spec.rb b/spec/unit/cookbook/synchronizer_spec.rb
index 3f5624f3b0..82876273e7 100644
--- a/spec/unit/cookbook/synchronizer_spec.rb
+++ b/spec/unit/cookbook/synchronizer_spec.rb
@@ -414,6 +414,13 @@ describe Chef::CookbookSynchronizer do
       and_return("/file-cache/cookbooks/cookbook_a/templates/default/apache2.conf.erb")
   end
 
+  describe "#server_api" do
+    it "sets keepalive to true" do
+      expect(Chef::ServerAPI).to receive(:new).with(Chef::Config[:chef_server_url], keepalives: true)
+      synchronizer.server_api
+    end
+  end
+
   describe "when syncing cookbooks with the server" do
     let(:server_api) { double("Chef::ServerAPI (mock)") }
 
diff --git a/spec/unit/http/basic_client_spec.rb b/spec/unit/http/basic_client_spec.rb
index 8cf63d4441..0def00a220 100644
--- a/spec/unit/http/basic_client_spec.rb
+++ b/spec/unit/http/basic_client_spec.rb
@@ -29,6 +29,26 @@ describe "HTTP Connection" do
     end
   end
 
+  describe "#initialize" do
+    it "calls .start when doing keepalives" do
+      basic_client = Chef::HTTP::BasicClient.new(uri, keepalives: true)
+      expect(basic_client).to receive(:configure_ssl)
+      net_http_mock = instance_double(Net::HTTP, proxy_address: nil, "proxy_port=" => nil, "read_timeout=" => nil, "open_timeout=" => nil)
+      expect(net_http_mock).to receive(:start).and_return(net_http_mock)
+      expect(Net::HTTP).to receive(:new).and_return(net_http_mock)
+      expect(basic_client.http_client).to eql(net_http_mock)
+    end
+
+    it "does not call .start when not doing keepalives" do
+      basic_client = Chef::HTTP::BasicClient.new(uri)
+      expect(basic_client).to receive(:configure_ssl)
+      net_http_mock = instance_double(Net::HTTP, proxy_address: nil, "proxy_port=" => nil, "read_timeout=" => nil, "open_timeout=" => nil)
+      expect(net_http_mock).not_to receive(:start)
+      expect(Net::HTTP).to receive(:new).and_return(net_http_mock)
+      expect(basic_client.http_client).to eql(net_http_mock)
+    end
+  end
+
   describe "#build_http_client" do
     it "should build an http client" do
       subject.build_http_client
diff --git a/spec/unit/http_spec.rb b/spec/unit/http_spec.rb
index 69e59ecaf8..d99f03aac9 100644
--- a/spec/unit/http_spec.rb
+++ b/spec/unit/http_spec.rb
@@ -43,6 +43,20 @@ describe Chef::HTTP do
 
   end
 
+  describe "#intialize" do
+    it "accepts a keepalive option and passes it to the http_client" do
+      http = Chef::HTTP.new(uri, keepalives: true)
+      expect(Chef::HTTP::BasicClient).to receive(:new).with(uri, ssl_policy: Chef::HTTP::APISSLPolicy, keepalives: true).and_call_original
+      expect(http.http_client).to be_a_kind_of(Chef::HTTP::BasicClient)
+    end
+
+    it "the default is not to use keepalives" do
+      http = Chef::HTTP.new(uri)
+      expect(Chef::HTTP::BasicClient).to receive(:new).with(uri, ssl_policy: Chef::HTTP::APISSLPolicy, keepalives: false).and_call_original
+      expect(http.http_client).to be_a_kind_of(Chef::HTTP::BasicClient)
+    end
+  end
+
   describe "create_url" do
 
     it "should return a correctly formatted url 1/3 CHEF-5261" do