From af3f3ae93ba1e43ac8231e4cf4e5a1f0ed8f3acb Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Wed, 11 Feb 2009 16:49:40 -0600
Subject: Refactoring the XMLRPC::Client error-handling
I split it all into smaller, manageable chunks,
and used methods for each step, instead
of having one huge call.
Note that I made all of the tests first, then
refactored the code, so I'm confident there's no
behavior change.
I don't know that this is actually a lot cleaner,
but it seems that way to me. I'm open to
skipping this, but I think it makes the whole thing
a lot cleaner.
Signed-off-by: Luke Kanies <luke@madstop.com>
---
spec/unit/network/xmlrpc/client.rb | 150 +++++++++++++++++++++++++++++++++++--
1 file changed, 143 insertions(+), 7 deletions(-)
(limited to 'spec/unit/network')
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
index a0a2e77fb..76ef5c799 100755
--- a/spec/unit/network/xmlrpc/client.rb
+++ b/spec/unit/network/xmlrpc/client.rb
@@ -2,12 +2,148 @@
Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
-describe Puppet::Network do
- it "should raise an XMLRPCClientError if a generated class raises a Timeout::Error" do
- http = mock 'http'
- Puppet::Network::HttpPool.stubs(:http_instance).returns http
- file = Puppet::Network::Client.file.new({:Server => "foo.com"})
- http.stubs(:post2).raises Timeout::Error
- lambda { file.retrieve }.should raise_error(Puppet::Network::XMLRPCClientError)
+describe Puppet::Network::XMLRPCClient do
+ describe "when performing the rpc call" do
+ before do
+ @client = Puppet::Network::Client.report.xmlrpc_client.new
+ @client.stubs(:call).returns "foo"
+
+ end
+
+ it "should call the specified namespace and method, with the specified arguments" do
+ @client.expects(:call).with("puppetreports.report", "eh").returns "foo"
+ @client.report("eh")
+ end
+
+ it "should return the results from the call" do
+ @client.expects(:call).returns "foo"
+ @client.report("eh").should == "foo"
+ end
+
+ describe "when returning the http instance" do
+ it "should use the http pool to create the instance" do
+ @client.instance_variable_set("@http", nil)
+ @client.expects(:host).returns "myhost"
+ @client.expects(:port).returns "myport"
+ Puppet::Network::HttpPool.expects(:http_instance).with("myhost", "myport", true).returns "http"
+
+ @client.http.should == "http"
+ end
+
+ it "should reuse existing instances" do
+ @client.http.should equal(@client.http)
+ end
+ end
+
+ describe "when recycling the connection" do
+ it "should close the existing instance if it's open" do
+ http = mock 'http'
+ @client.stubs(:http).returns http
+
+ http.expects(:started?).returns true
+ http.expects(:finish)
+
+ @client.recycle_connection
+ end
+
+ it "should force creation of a new instance" do
+ Puppet::Network::HttpPool.expects(:http_instance).returns "second_http"
+
+ @client.recycle_connection
+
+ @client.http.should == "second_http"
+ end
+ end
+
+ describe "and an exception is raised" do
+ it "should raise XMLRPCClientError if XMLRPC::FaultException is raised" do
+ error = XMLRPC::FaultException.new("foo", "bar")
+
+ @client.expects(:call).raises(error)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should raise XMLRPCClientError if Errno::ECONNREFUSED is raised" do
+ @client.expects(:call).raises(Errno::ECONNREFUSED)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if Timeout::Error is raised" do
+ Puppet.expects(:err)
+ @client.expects(:call).raises(Timeout::Error)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if SocketError is raised" do
+ Puppet.expects(:err)
+ @client.expects(:call).raises(SocketError)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log, recycle the connection, and retry if Errno::EPIPE is raised" do
+ @client.expects(:call).times(2).raises(Errno::EPIPE).then.returns "eh"
+
+ Puppet.expects(:warning)
+ @client.expects(:recycle_connection)
+
+ @client.report("eh")
+ end
+
+ it "should log, recycle the connection, and retry if EOFError is raised" do
+ @client.expects(:call).times(2).raises(EOFError).then.returns "eh"
+
+ Puppet.expects(:warning)
+ @client.expects(:recycle_connection)
+
+ @client.report("eh")
+ end
+
+ it "should log and retry if an exception containing 'Wrong size' is raised" do
+ error = RuntimeError.new("Wrong size. Was 15, should be 30")
+ @client.expects(:call).times(2).raises(error).then.returns "eh"
+
+ Puppet.expects(:warning)
+
+ @client.report("eh")
+ end
+
+ it "should raise XMLRPCClientError if OpenSSL::SSL::SSLError is raised" do
+ @client.expects(:call).raises(OpenSSL::SSL::SSLError)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if OpenSSL::SSL::SSLError is raised with certificate issues" do
+ error = OpenSSL::SSL::SSLError.new("hostname was not match")
+ @client.expects(:call).raises(error)
+
+ Puppet.expects(:warning)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log, recycle the connection, and retry if OpenSSL::SSL::SSLError is raised containing 'bad write retry'" do
+ error = OpenSSL::SSL::SSLError.new("bad write retry")
+ @client.expects(:call).times(2).raises(error).then.returns "eh"
+
+ @client.expects(:recycle_connection)
+
+ Puppet.expects(:warning)
+
+ @client.report("eh")
+ end
+
+ it "should log and raise XMLRPCClientError if any other exception is raised" do
+ @client.expects(:call).raises(RuntimeError)
+
+ Puppet.expects(:err)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+ end
end
end
--
cgit
From 5e35166f1b7219d470916d1ee7a4e6852afaf1f9 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Wed, 11 Feb 2009 16:54:04 -0600
Subject: Fixing #961 - closing the http connection after every xmlrpc call
There were apparently some circumstances that
resulted in the connection not being closed; this just closes
it every time if it's still open after the rpc call is complete.
Signed-off-by: Luke Kanies <luke@madstop.com>
---
spec/unit/network/xmlrpc/client.rb | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
(limited to 'spec/unit/network')
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
index 76ef5c799..36e59429c 100755
--- a/spec/unit/network/xmlrpc/client.rb
+++ b/spec/unit/network/xmlrpc/client.rb
@@ -20,6 +20,28 @@ describe Puppet::Network::XMLRPCClient do
@client.report("eh").should == "foo"
end
+ it "should always close the http connection if it is still open after the call" do
+ http = mock 'http'
+ @client.stubs(:http).returns http
+
+ http.expects(:started?).returns true
+ http.expects(:finish)
+
+ @client.report("eh").should == "foo"
+ end
+
+ it "should always close the http connection if it is still open after a call that raises an exception" do
+ http = mock 'http'
+ @client.stubs(:http).returns http
+
+ @client.expects(:call).raises RuntimeError
+
+ http.expects(:started?).returns true
+ http.expects(:finish)
+
+ lambda { @client.report("eh") }.should raise_error
+ end
+
describe "when returning the http instance" do
it "should use the http pool to create the instance" do
@client.instance_variable_set("@http", nil)
--
cgit