Disabling http keep-alive as a means of preventing #1010.

There is now a constant in Puppet::Network::HttpPool that will
disable or enable this feature, but note that we determined
that it can cause corruption, especially in file serving (but
it's client-side corruption).

5 files changed, 188 insertions, 105 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 43103252f..09e1ca533 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,9 @@
+    Disabling http keep-alive as a means of preventing #1010.
+    There is now a constant in Puppet::Network::HttpPool that will
+    disable or enable this feature, but note that we determined
+    that it can cause corruption, especially in file serving (but
+    it's client-side corruption).
+
     Applying patch by Ryan McBride to fix OpenBSD package 
     matching.  The actual problem was caused by the fix to #1001.
 
diff --git a/lib/puppet/network/client.rb b/lib/puppet/network/client.rb
index cf1782f79..478883959 100644
--- a/lib/puppet/network/client.rb
+++ b/lib/puppet/network/client.rb
@@ -96,8 +96,9 @@ class Puppet::Network::Client
             self.read_cert
 
             # We have to start the HTTP connection manually before we start
-            # sending it requests or keep-alive won't work.
-            @driver.start if @driver.respond_to? :start
+            # sending it requests or keep-alive won't work.  Note that with #1010,
+            # we don't currently actually want keep-alive.
+            @driver.start if @driver.respond_to? :start and Puppet::Network::HttpPool.keep_alive?
 
             @local = false
         elsif hash.include?(driverparam)
diff --git a/lib/puppet/network/http_pool.rb b/lib/puppet/network/http_pool.rb
index 69574d8fd..9d37f2eeb 100644
--- a/lib/puppet/network/http_pool.rb
+++ b/lib/puppet/network/http_pool.rb
@@ -6,6 +6,15 @@ end
 
 # Manage Net::HTTP instances for keep-alive.
 module Puppet::Network::HttpPool
+    # 2008/03/23
+    # LAK:WARNING: Enabling this has a high propability of
+    # causing corrupt files and who knows what else.  See #1010.
+    HTTP_KEEP_ALIVE = false
+
+    def self.keep_alive?
+        HTTP_KEEP_ALIVE
+    end
+
     # This handles reading in the key and such-like.
     extend Puppet::SSLCertificates::Support
     @http_cache = {}
@@ -56,12 +65,14 @@ module Puppet::Network::HttpPool
 
         # Return our cached instance if we've got a cache, as long as we're not
         # resetting the instance.
-        return @http_cache[key] if ! reset 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?
+        if keep_alive?
+            return @http_cache[key] if ! reset 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
         end
 
         args = [host, port]
@@ -88,7 +99,7 @@ module Puppet::Network::HttpPool
 
         cert_setup(http)
 
-        @http_cache[key] = http
+        @http_cache[key] = http if keep_alive?
 
         return http
     end
diff --git a/spec/unit/network/client.rb b/spec/unit/network/client.rb
new file mode 100644
index 000000000..bc41efb4f
--- /dev/null
+++ b/spec/unit/network/client.rb
@@ -0,0 +1,43 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2008-3-24.
+#  Copyright (c) 2008. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/network/client'
+
+describe Puppet::Network::Client do
+    before do
+        Puppet::Network::HttpPool.stubs(:cert_setup)
+    end
+    describe "when keep-alive is enabled" do
+        before do
+            Puppet::Network::HttpPool.stubs(:keep_alive?).returns true
+        end
+        it "should start the http client up on creation" do
+            http = mock 'http'
+            http.stub_everything
+            http.expects(:start)
+            Net::HTTP.stubs(:new).returns http
+
+            # Pick a random subclass...
+            Puppet::Network::Client.master.new :Server => Puppet[:server]
+        end
+    end
+
+    describe "when keep-alive is disabled" do
+        before do
+            Puppet::Network::HttpPool.stubs(:keep_alive?).returns false
+        end
+        it "should not start the http client up on creation" do
+            http = mock 'http'
+            http.stub_everything
+            http.expects(:start).never
+            Net::HTTP.stubs(:new).returns http
+
+            # Pick a random subclass...
+            Puppet::Network::Client.master.new :Server => Puppet[:server]
+        end
+    end
+end
diff --git a/spec/unit/network/http_pool.rb b/spec/unit/network/http_pool.rb
index 3043c5e61..1fbc17471 100755
--- a/spec/unit/network/http_pool.rb
+++ b/spec/unit/network/http_pool.rb
@@ -14,6 +14,10 @@ describe Puppet::Network::HttpPool, " when adding certificate information to htt
         [:add_file,:purpose=].each { |m| @store.stubs(m) }
     end
 
+    it "should have keep-alive disabled" do
+        Puppet::Network::HttpPool::HTTP_KEEP_ALIVE.should be_false
+    end
+
     it "should do nothing if no certificate is available" do
         Puppet::Network::HttpPool.expects(:read_cert).returns(false)
         @http.expects(:cert=).never
@@ -102,117 +106,135 @@ describe Puppet::Network::HttpPool, " when adding certificate information to htt
     after do
         Puppet::Network::HttpPool.clear_http_instances
     end
-end
 
-describe Puppet::Network::HttpPool, " when managing http instances" do
-    def stub_settings(settings)
-        settings.each do |param, value|
-            Puppet.settings.stubs(:value).with(param).returns(value)
+    describe "when managing http instances" do
+        def stub_settings(settings)
+            settings.each do |param, value|
+                Puppet.settings.stubs(:value).with(param).returns(value)
+            end
         end
-    end
-
-    before do
-        # All of hte cert stuff is tested elsewhere
-        Puppet::Network::HttpPool.stubs(:cert_setup)
-    end
-
-    it "should return an http instance created with the passed host and port" do
-        http = stub 'http', :use_ssl= => nil, :read_timeout= => nil, :open_timeout= => nil, :enable_post_connection_check= => nil, :started? => false
-        Net::HTTP.expects(:new).with("me", 54321, nil, nil).returns(http)
-        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(http)
-    end
-
-    it "should enable ssl on the http instance" do
-        Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@use_ssl").should be_true
-    end
 
-    it "should set the read timeout" do
-        Puppet::Network::HttpPool.http_instance("me", 54321).read_timeout.should == 120
-    end
+        before do
+            # All of hte cert stuff is tested elsewhere
+            Puppet::Network::HttpPool.stubs(:cert_setup)
+        end
 
-    it "should set the open timeout" do
-        Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
-    end
+        it "should return an http instance created with the passed host and port" do
+            http = stub 'http', :use_ssl= => nil, :read_timeout= => nil, :open_timeout= => nil, :enable_post_connection_check= => nil, :started? => false
+            Net::HTTP.expects(:new).with("me", 54321, nil, nil).returns(http)
+            Puppet::Network::HttpPool.http_instance("me", 54321).should equal(http)
+        end
 
-    it "should default to http_enable_post_connection_check being enabled" do
-        Puppet.settings[:http_enable_post_connection_check].should be_true
-    end
+        it "should enable ssl on the http instance" do
+            Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@use_ssl").should be_true
+        end
 
-    # JJM: I'm not sure if this is correct, as this really follows the
-    # configuration option.
-    it "should set enable_post_connection_check true " do
-        Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@enable_post_connection_check").should be_true
-    end
+        it "should set the read timeout" do
+            Puppet::Network::HttpPool.http_instance("me", 54321).read_timeout.should == 120
+        end
 
-    it "should create the http instance with the proxy host and port set if the http_proxy is not set to 'none'" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
-    end
+        it "should set the open timeout" do
+            Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
+        end
 
-    it "should cache http instances" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::HttpPool.http_instance("me", 54321)
-        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
-    end
+        it "should default to http_enable_post_connection_check being enabled" do
+            Puppet.settings[:http_enable_post_connection_check].should be_true
+        end
 
-    it "should have a mechanism for getting a new http instance instead of the cached instance" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::HttpPool.http_instance("me", 54321)
-        Puppet::Network::HttpPool.http_instance("me", 54321, true).should_not equal(old)
-    end
+        # JJM: I'm not sure if this is correct, as this really follows the
+        # configuration option.
+        it "should set enable_post_connection_check true " do
+            Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@enable_post_connection_check").should be_true
+        end
 
-    it "should close existing, open connections when requesting a new connection" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::HttpPool.http_instance("me", 54321)
-        old.expects(:started?).returns(true)
-        old.expects(:finish)
-        Puppet::Network::HttpPool.http_instance("me", 54321, true)
-    end
+        it "should create the http instance with the proxy host and port set if the http_proxy is not set to 'none'" do
+            stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+            Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
+        end
 
-    it "should have a mechanism for clearing the http cache" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::HttpPool.http_instance("me", 54321)
-        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
-        old = Puppet::Network::HttpPool.http_instance("me", 54321)
-        Puppet::Network::HttpPool.clear_http_instances
-        Puppet::Network::HttpPool.http_instance("me", 54321).should_not equal(old)
-    end
+        describe "when http keep-alive is enabled" do
+            before do
+                Puppet::Network::HttpPool.stubs(:keep_alive?).returns true
+            end
+
+            it "should cache http instances" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
+            end
+
+            it "should have a mechanism for getting a new http instance instead of the cached instance" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                Puppet::Network::HttpPool.http_instance("me", 54321, true).should_not equal(old)
+            end
+
+            it "should close existing, open connections when requesting a new connection" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                old.expects(:started?).returns(true)
+                old.expects(:finish)
+                Puppet::Network::HttpPool.http_instance("me", 54321, true)
+            end
+
+            it "should have a mechanism for clearing the http cache" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                Puppet::Network::HttpPool.clear_http_instances
+                Puppet::Network::HttpPool.http_instance("me", 54321).should_not equal(old)
+            end
+
+            it "should close open http connections when clearing the cache" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                one = Puppet::Network::HttpPool.http_instance("me", 54321)
+                one.expects(:started?).returns(true)
+                one.expects(:finish).returns(true)
+                Puppet::Network::HttpPool.clear_http_instances
+            end
+
+            it "should not close unopened http connections when clearing the cache" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                one = Puppet::Network::HttpPool.http_instance("me", 54321)
+                one.expects(:started?).returns(false)
+                one.expects(:finish).never
+                Puppet::Network::HttpPool.clear_http_instances
+            end
+        end
 
-    it "should close open http connections when clearing the cache" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        one = Puppet::Network::HttpPool.http_instance("me", 54321)
-        one.expects(:started?).returns(true)
-        one.expects(:finish).returns(true)
-        Puppet::Network::HttpPool.clear_http_instances
-    end
+        describe "when http keep-alive is disabled" do
+            before do
+                Puppet::Network::HttpPool.stubs(:keep_alive?).returns false
+            end
 
-    it "should not close unopened http connections when clearing the cache" do
-        stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        one = Puppet::Network::HttpPool.http_instance("me", 54321)
-        one.expects(:started?).returns(false)
-        one.expects(:finish).never
-        Puppet::Network::HttpPool.clear_http_instances
-    end
+            it "should not cache http instances" do
+                stub_settings :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+                old = Puppet::Network::HttpPool.http_instance("me", 54321)
+                Puppet::Network::HttpPool.http_instance("me", 54321).should_not equal(old)
+            end
+        end
 
-    # We mostly have to do this for testing, since in real life people
-    # won't change certs within a single process.
-    it "should remove its loaded certificate when clearing the cache" do
-        Puppet::Network::HttpPool.instance_variable_set("@cert", :yay)
-        Puppet::Network::HttpPool.clear_http_instances
-        # Can't use the accessor, because it will read the cert in
-        Puppet::Network::HttpPool.instance_variable_get("@cert").should be_nil
-    end
+        # We mostly have to do this for testing, since in real life people
+        # won't change certs within a single process.
+        it "should remove its loaded certificate when clearing the cache" do
+            Puppet::Network::HttpPool.instance_variable_set("@cert", :yay)
+            Puppet::Network::HttpPool.clear_http_instances
+            # Can't use the accessor, because it will read the cert in
+            Puppet::Network::HttpPool.instance_variable_get("@cert").should be_nil
+        end
 
-    # We mostly have to do this for testing, since in real life people
-    # won't change certs within a single process.
-    it "should remove its loaded key when clearing the cache" do
-        Puppet::Network::HttpPool.instance_variable_set("@key", :yay)
-        Puppet::Network::HttpPool.clear_http_instances
-        # Can't use the accessor, because it will read the cert in
-        Puppet::Network::HttpPool.instance_variable_get("@key").should be_nil
-    end
+        # We mostly have to do this for testing, since in real life people
+        # won't change certs within a single process.
+        it "should remove its loaded key when clearing the cache" do
+            Puppet::Network::HttpPool.instance_variable_set("@key", :yay)
+            Puppet::Network::HttpPool.clear_http_instances
+            # Can't use the accessor, because it will read the cert in
+            Puppet::Network::HttpPool.instance_variable_get("@key").should be_nil
+        end
 
-    after do
-        Puppet::Network::HttpPool.clear_http_instances
+        after do
+            Puppet::Network::HttpPool.clear_http_instances
+        end
     end
 end