THRIFT-4999: Raise proper exception on transport error

Client: ruby
Patch: Grégoire Seux

This closes #1924

Before this patch, any error on the http layer was ignored and usually
seen by the user as ProtocolException instead of TransportException

2 files changed, 24 insertions, 0 deletions

diff --git a/lib/rb/lib/thrift/transport/http_client_transport.rb b/lib/rb/lib/thrift/transport/http_client_transport.rb
index 5c1dd5c8a..c84304d99 100644
--- a/lib/rb/lib/thrift/transport/http_client_transport.rb
+++ b/lib/rb/lib/thrift/transport/http_client_transport.rb
@@ -47,6 +47,8 @@ module Thrift
       http.use_ssl = @url.scheme == 'https'
       http.verify_mode = @ssl_verify_mode if @url.scheme == 'https'
       resp = http.post(@url.request_uri, @outbuf, @headers)
+      raise TransportException.new(TransportException::UNKNOWN, "#{self.class.name} Could not connect to #{@url}, HTTP status code #{resp.code.to_i}") unless (200..299).include?(resp.code.to_i)
+
       data = resp.body
       data = Bytes.force_binary_encoding(data)
       @inbuf = StringIO.new data
diff --git a/lib/rb/spec/http_client_spec.rb b/lib/rb/spec/http_client_spec.rb
index df472ab33..292c7521e 100644
--- a/lib/rb/spec/http_client_spec.rb
+++ b/lib/rb/spec/http_client_spec.rb
@@ -45,6 +45,7 @@ describe 'Thrift::HTTPClientTransport' do
           expect(http).to receive(:post).with("/path/to/service?param=value", "a test frame", {"Content-Type"=>"application/x-thrift"}) do
             double("Net::HTTPOK").tap do |response|
               expect(response).to receive(:body).and_return "data"
+              expect(response).to receive(:code).and_return "200"
             end
           end
         end
@@ -65,6 +66,7 @@ describe 'Thrift::HTTPClientTransport' do
           expect(http).to receive(:post).with("/path/to/service?param=value", "test", headers) do
             double("Net::HTTPOK").tap do |response|
               expect(response).to receive(:body).and_return "data"
+              expect(response).to receive(:code).and_return "200"
             end
           end
         end
@@ -86,6 +88,24 @@ describe 'Thrift::HTTPClientTransport' do
       expect(@client.instance_variable_get(:@outbuf)).to eq(Thrift::Bytes.empty_byte_buffer)
     end
 
+    it 'should raise TransportError on HTTP failures' do
+      @client.write "test"
+
+      expect(Net::HTTP).to receive(:new).with("my.domain.com", 80) do
+        double("Net::HTTP").tap do |http|
+          expect(http).to receive(:use_ssl=).with(false)
+          expect(http).to receive(:post).with("/path/to/service?param=value", "test", {"Content-Type"=>"application/x-thrift"}) do
+            double("Net::HTTPOK").tap do |response|
+              expect(response).not_to receive(:body)
+              expect(response).to receive(:code).at_least(:once).and_return "503"
+            end
+          end
+        end
+      end
+
+      expect { @client.flush }.to raise_error(Thrift::TransportException)
+    end
+
   end
 
   describe 'ssl enabled' do
@@ -107,6 +127,7 @@ describe 'Thrift::HTTPClientTransport' do
               "Content-Type" => "application/x-thrift") do
             double("Net::HTTPOK").tap do |response|
               expect(response).to receive(:body).and_return "data"
+              expect(response).to receive(:code).and_return "200"
             end
           end
         end
@@ -128,6 +149,7 @@ describe 'Thrift::HTTPClientTransport' do
               "Content-Type" => "application/x-thrift") do
             double("Net::HTTPOK").tap do |response|
               expect(response).to receive(:body).and_return "data"
+              expect(response).to receive(:code).and_return "200"
             end
           end
         end