Entirely refactoring http keep-alive. There's now

a central module responsible for managing the http pool
(Puppet::Network::HttpPool), and it also handles
setting certificate information.  This gets rid of
what were otherwise long chains of method calls,
and it makes the code paths much clearer.

4 files changed, 109 insertions, 96 deletions

diff --git a/lib/puppet/network/client.rb b/lib/puppet/network/client.rb
index 52431e227..9a861fdf2 100644
--- a/lib/puppet/network/client.rb
+++ b/lib/puppet/network/client.rb
@@ -122,13 +122,8 @@ class Puppet::Network::Client
     end
 
     # Make sure we set the driver up when we read the cert in.
-    def read_cert
-        if super
-            @driver.recycle_connection(self) if @driver.respond_to?(:recycle_connection)
-            return true
-        else
-            return false
-        end
+    def recycle_connection
+        @driver.recycle_connection if @driver.respond_to?(:recycle_connection)
     end
 
     # A wrapper method to run and then store the last run time
@@ -141,9 +136,7 @@ class Puppet::Network::Client
             self.run
             self.lastrun = Time.now.to_i
         rescue => detail
-            if Puppet[:trace]
-                puts detail.backtrace
-            end
+            puts detail.backtrace if Puppet[:trace]
             Puppet.err "Could not run %s: %s" % [self.class, detail]
         end
     end
diff --git a/lib/puppet/network/client/master.rb b/lib/puppet/network/client/master.rb
index 341192740..8d768018f 100644
--- a/lib/puppet/network/client/master.rb
+++ b/lib/puppet/network/client/master.rb
@@ -1,6 +1,7 @@
 # The client for interacting with the puppetmaster config server.
 require 'sync'
 require 'timeout'
+require 'puppet/network/http_pool'
 
 class Puppet::Network::Client::Master < Puppet::Network::Client
     unless defined? @@sync
@@ -274,7 +275,7 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
 
                 # Now close all of our existing http connections, since there's no
                 # reason to leave them lying open.
-                Puppet::Network::XMLRPCClient.clear_http_instances
+                Puppet::Network::HttpPool.clear_http_instances
             end
             
             lockfile.unlock
diff --git a/lib/puppet/network/http_pool.rb b/lib/puppet/network/http_pool.rb
new file mode 100644
index 000000000..f6038f189
--- /dev/null
+++ b/lib/puppet/network/http_pool.rb
@@ -0,0 +1,92 @@
+require 'puppet/sslcertificates/support'
+require 'net/https'
+
+# Manage Net::HTTP instances for keep-alive.
+module Puppet::Network::HttpPool
+    # This handles reading in the key and such-like.
+    extend Puppet::SSLCertificates::Support
+    @http_cache = {}
+
+    # Clear our http cache, closing all connections.
+    def self.clear_http_instances
+        @http_cache.each do |name, connection|
+            connection.finish if connection.started?
+        end
+        @http_cache.clear
+        @cert = nil
+        @key = nil
+    end
+
+    # Make sure we set the driver up when we read the cert in.
+    def self.read_cert
+        if val = super # This calls read_cert from the Puppet::SSLCertificates::Support module.
+            # Clear out all of our connections, since they previously had no cert and now they
+            # should have them.
+            clear_http_instances
+            return val
+        else
+            return false
+        end
+    end
+
+    # Use cert information from a Puppet client to set up the http object.
+    def self.cert_setup(http)
+        # Just no-op if we don't have certs.
+        return false unless (defined?(@cert) and @cert) or self.read_cert
+
+        store = OpenSSL::X509::Store.new
+        store.add_file Puppet[:localcacert]
+        store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
+
+        http.cert_store = store
+        http.ca_file = Puppet[:localcacert]
+        http.cert = self.cert
+        http.verify_mode = OpenSSL::SSL::VERIFY_PEER
+        http.key = self.key
+    end
+
+    # Retrieve a cached http instance of caching is enabled, else return
+    # a new one.
+    def self.http_instance(host, port, reset = false)
+        # We overwrite the uninitialized @http here with a cached one.
+        key = "%s:%s" % [host, port]
+
+        # Return our cached instance if keepalive is enabled and we've got
+        # a cache, as long as we're not resetting the instance.
+        return @http_cache[key] if ! reset and Puppet[:http_keepalive] and @http_cache[key]
+
+        # Clean up old connections if we have them.
+        if http = @http_cache[key]
+            @http_cache.delete(key)
+            http.finish if http.started?
+        end
+
+        args = [host, port]
+        if Puppet[:http_proxy_host] == "none"
+            args << nil << nil
+        else
+            args << Puppet[:http_proxy_host] << Puppet[:http_proxy_port]
+        end
+        http = Net::HTTP.new(*args)
+
+        # Pop open the http client a little; older versions of Net::HTTP(s) didn't
+        # give us a reader for ca_file... Grr...
+        class << http; attr_accessor :ca_file; end
+
+        http.use_ssl = true
+        http.read_timeout = 120
+        http.open_timeout = 120
+        # JJM Configurable fix for #896.
+        if Puppet[:http_enable_post_connection_check]
+            http.enable_post_connection_check = true
+        else
+            http.enable_post_connection_check = false
+        end
+
+        cert_setup(http)
+
+        @http_cache[key] = http if Puppet[:http_keepalive]
+
+        return http
+    end
+end
diff --git a/lib/puppet/network/xmlrpc/client.rb b/lib/puppet/network/xmlrpc/client.rb
index 5048a040a..27bb3dc5e 100644
--- a/lib/puppet/network/xmlrpc/client.rb
+++ b/lib/puppet/network/xmlrpc/client.rb
@@ -1,4 +1,5 @@
 require 'puppet/sslcertificates'
+require 'puppet/network/http_pool'
 require 'openssl'
 require 'puppet/external/base64'
 
@@ -10,66 +11,15 @@ module Puppet::Network
     class ClientError < Puppet::Error; end
     class XMLRPCClientError < Puppet::Error; end
     class XMLRPCClient < ::XMLRPC::Client
+
         attr_accessor :puppet_server, :puppet_port
         @clients = {}
-        @@http_cache = {}
 
         class << self
             include Puppet::Util
             include Puppet::Util::ClassGen
         end
 
-        # Clear our http cache, closing all connections.
-        def self.clear_http_instances
-            @@http_cache.each do |name, connection|
-                connection.finish if connection.started?
-            end
-            @@http_cache.clear
-        end
-
-        # Retrieve a cached http instance of caching is enabled, else return
-        # a new one.
-        def self.http_instance(host, port, reset = false)
-            # We overwrite the uninitialized @http here with a cached one.
-            key = "%s:%s" % [host, port]
-
-            # Return our cached instance if keepalive is enabled and we've got
-            # a cache, as long as we're not resetting the instance.
-            return @@http_cache[key] if ! reset and Puppet[:http_keepalive] and @@http_cache[key]
-
-            # Clean up old connections if we have them.
-            if http = @@http_cache[key]
-                @@http_cache.delete(key)
-                http.finish if http.started?
-            end
-
-            args = [host, port]
-            if Puppet[:http_proxy_host] == "none"
-                args << nil << nil
-            else
-                args << Puppet[:http_proxy_host] << Puppet[:http_proxy_port]
-            end
-            http = Net::HTTP.new(*args)
-
-            # Pop open the http client a little; older versions of Net::HTTP(s) didn't
-            # give us a reader for ca_file... Grr...
-            class << http; attr_accessor :ca_file; end
-
-            http.use_ssl = true
-            http.read_timeout = 120
-            http.open_timeout = 120
-            # JJM Configurable fix for #896.
-            if Puppet[:http_enable_post_connection_check]
-                http.enable_post_connection_check = true
-            else
-                http.enable_post_connection_check = false
-            end
-
-            @@http_cache[key] = http if Puppet[:http_keepalive]
-
-            return http
-        end
-
         # Create a netclient for each handler
         def self.mkclient(handler)
             interface = handler.interface
@@ -81,8 +31,7 @@ module Puppet::Network
             # they want.
             constant = handler.name.to_s.capitalize
             name = namespace.downcase
-            newclient = genclass(name, :hash => @clients,
-                :constant => constant)
+            newclient = genclass(name, :hash => @clients, :constant => constant)
 
             interface.methods.each { |ary|
                 method = ary[0]
@@ -97,7 +46,7 @@ module Puppet::Network
                     rescue OpenSSL::SSL::SSLError => detail
                         if detail.message =~ /bad write retry/
                             Puppet.warning "Transient SSL write error; restarting connection and retrying"
-                            self.recycle_connection(@cert_client)
+                            self.recycle_connection
                             retry
                         end
                         raise XMLRPCClientError,
@@ -118,7 +67,7 @@ module Puppet::Network
                         raise error
                     rescue Errno::EPIPE, EOFError
                         Puppet.warning "Other end went away; restarting connection and retrying"
-                        self.recycle_connection(@cert_client)
+                        self.recycle_connection
                         retry
                     rescue => detail
                         if detail.message =~ /^Wrong size\. Was \d+, should be \d+$/
@@ -141,30 +90,6 @@ module Puppet::Network
             @clients[handler] || self.mkclient(handler)
         end
 
-        # Use cert information from a Puppet client to set up the http object.
-        def cert_setup(client)
-            # Cache it for next time
-            @cert_client = client
-            
-            unless FileTest.exist?(Puppet[:localcacert])
-                raise Puppet::SSLCertificates::Support::MissingCertificate,
-                    "Could not find ca certificate %s" % Puppet[:localcacert]
-            end
-
-            # We can't overwrite certificates, @http will freeze itself
-            # once started.
-            unless @http.ca_file
-                @http.ca_file = Puppet[:localcacert]
-                store = OpenSSL::X509::Store.new
-                store.add_file Puppet[:localcacert]
-                store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
-                @http.cert_store = store
-                @http.cert = client.cert
-                @http.verify_mode = OpenSSL::SSL::VERIFY_PEER
-                @http.key = client.key
-            end
-        end
-
         def initialize(hash = {})
             hash[:Path] ||= "/RPC2"
             hash[:Server] ||= Puppet[:server]
@@ -188,13 +113,15 @@ module Puppet::Network
                 true, # use_ssl
                 120 # a two minute timeout, instead of 30 seconds
             )
-            @http = self.class.http_instance(@host, @port)
+            @http = Puppet::Network::HttpPool.http_instance(@host, @port)
         end
  
-        def recycle_connection(client)
-            @http = self.class.http_instance(@host, @port, true) # reset the instance
-
-            cert_setup(client)
+        # Get rid of our existing connection, replacing it with a new one.
+        # This should only happen if we lose our connection somehow (e.g., an EPIPE)
+        # or we've just downloaded certs and we need to create new http instances
+        # with the certs added.
+        def recycle_connection
+            @http = Puppet::Network::HttpPool.http_instance(@host, @port, true) # reset the instance
         end
         
         def start