Add support for Net::HTTP#write_timeout method (Ruby 2.6.0) (#647)

* Add support for `Net::HTTP#write_timeout` method (Ruby 2.6.0)

[The method](https://ruby-doc.org/stdlib-2.6/libdoc/net/http/rdoc/Net/HTTP.html#method-i-write_timeout-3D) was introduced in Ruby 2.6.0.

The gem already supports `open_timeout` & `read_timeout`, so it would be nice to provide support for `write_timeout` as well.

From the official documentation:

> Number of seconds to wait for one block to be written (via one write(2) call). Any number may be used, including Floats for fractional seconds. If the HTTP object cannot write data in this many seconds, it raises a Net::WriteTimeout exception. The default value is 60 seconds. Net::WriteTimeout is not raised on Windows.

* Introduce private `validate_timeout_argument` method to reduce duplication

* Introduce private `add_timeout?` method to reduce duplication

* Do not warn about `write_timeout` when set using the default `timeout` option

* Introduce private `from_ruby_version` method to reduce duplication

.travis.yml

@@ -6,6 +6,6 @@ rvm:
   - 2.3.8
   - 2.4.5
   - 2.5.3
-  - 2.6.0
+  - 2.6.1
 bundler_args: --without development
 before_install: gem install bundler -v '< 2'

lib/httparty.rb

@@ -173,9 +173,9 @@ module HTTParty
     #     include HTTParty
     #     default_timeout 10
     #   end
-    def default_timeout(t)
-      raise ArgumentError, 'Timeout must be an integer or float' unless t && (t.is_a?(Integer) || t.is_a?(Float))
-      default_options[:timeout] = t
+    def default_timeout(value)
+      validate_timeout_argument(__method__, value)
+      default_options[:timeout] = value
     end
 
     # Allows setting a default open_timeout for all HTTP calls in seconds
@@ -184,9 +184,9 @@ module HTTParty
     #     include HTTParty
     #     open_timeout 10
     #   end
-    def open_timeout(t)
-      raise ArgumentError, 'open_timeout must be an integer or float' unless t && (t.is_a?(Integer) || t.is_a?(Float))
-      default_options[:open_timeout] = t
+    def open_timeout(value)
+      validate_timeout_argument(__method__, value)
+      default_options[:open_timeout] = value
     end
 
     # Allows setting a default read_timeout for all HTTP calls in seconds
@@ -195,11 +195,24 @@ module HTTParty
     #     include HTTParty
     #     read_timeout 10
     #   end
-    def read_timeout(t)
-      raise ArgumentError, 'read_timeout must be an integer or float' unless t && (t.is_a?(Integer) || t.is_a?(Float))
-      default_options[:read_timeout] = t
+    def read_timeout(value)
+      validate_timeout_argument(__method__, value)
+      default_options[:read_timeout] = value
     end
 
+    # Allows setting a default write_timeout for all HTTP calls in seconds
+    # Supported by Ruby > 2.6.0
+    #
+    #   class Foo
+    #     include HTTParty
+    #     write_timeout 10
+    #   end
+    def write_timeout(value)
+      validate_timeout_argument(__method__, value)
+      default_options[:write_timeout] = value
+    end
+
+
     # Set an output stream for debugging, defaults to $stderr.
     # The output stream is passed on to Net::HTTP#set_debug_output.
     #
@@ -563,6 +576,10 @@ module HTTParty
 
     private
 
+    def validate_timeout_argument(timeout_type, value)
+      raise ArgumentError, "#{ timeout_type } must be an integer or float" unless value && (value.is_a?(Integer) || value.is_a?(Float))
+    end
+
     def ensure_method_maintained_across_redirects(options)
       unless options.key?(:maintain_method_across_redirects)
         options[:maintain_method_across_redirects] = true

lib/httparty/connection_adapter.rb

@@ -42,6 +42,7 @@ module HTTParty
   # * :+timeout+: timeout in seconds
   # * :+open_timeout+: http connection open_timeout in seconds, overrides timeout if set
   # * :+read_timeout+: http connection read_timeout in seconds, overrides timeout if set
+  # * :+write_timeout+: http connection write_timeout in seconds, overrides timeout if set (Ruby >= 2.6.0 required)
   # * :+debug_output+: see HTTParty::ClassMethods.debug_output.
   # * :+cert_store+: contains certificate data. see method 'attach_ssl_certificates'
   # * :+pem+: contains pem client certificate data. see method 'attach_ssl_certificates'
@@ -113,19 +114,29 @@ module HTTParty
 
       attach_ssl_certificates(http, options)
 
-      if options[:timeout] && (options[:timeout].is_a?(Integer) || options[:timeout].is_a?(Float))
+      if add_timeout?(options[:timeout])
         http.open_timeout = options[:timeout]
         http.read_timeout = options[:timeout]
+
+        from_ruby_version('2.6.0', option: :write_timeout, warn: false) do
+          http.write_timeout = options[:timeout]
+        end
       end
 
-      if options[:read_timeout] && (options[:read_timeout].is_a?(Integer) || options[:read_timeout].is_a?(Float))
+      if add_timeout?(options[:read_timeout])
         http.read_timeout = options[:read_timeout]
       end
 
-      if options[:open_timeout] && (options[:open_timeout].is_a?(Integer) || options[:open_timeout].is_a?(Float))
+      if add_timeout?(options[:open_timeout])
         http.open_timeout = options[:open_timeout]
       end
 
+      if add_timeout?(options[:write_timeout])
+        from_ruby_version('2.6.0', option: :write_timeout) do
+          http.write_timeout = options[:write_timeout]
+        end
+      end
+
       if options[:debug_output]
         http.set_debug_output(options[:debug_output])
       end
@@ -138,18 +149,14 @@ module HTTParty
       #
       # @see https://bugs.ruby-lang.org/issues/6617
       if options[:local_host]
-        if RUBY_VERSION >= "2.0.0"
+        from_ruby_version('2.0.0', option: :local_host) do
           http.local_host = options[:local_host]
-        else
-          Kernel.warn("Warning: option :local_host requires Ruby version 2.0 or later")
         end
       end
 
       if options[:local_port]
-        if RUBY_VERSION >= "2.0.0"
+        from_ruby_version('2.0.0', option: :local_port) do
           http.local_port = options[:local_port]
-        else
-          Kernel.warn("Warning: option :local_port requires Ruby version 2.0 or later")
         end
       end
 
@@ -158,6 +165,18 @@ module HTTParty
 
     private
 
+    def from_ruby_version(ruby_version, option: nil, warn: true)
+      if RUBY_VERSION >= ruby_version
+        yield
+      elsif warn
+        Kernel.warn("Warning: option #{ option } requires Ruby version #{ ruby_version } or later")
+      end
+    end
+
+    def add_timeout?(timeout)
+      timeout && (timeout.is_a?(Integer) || timeout.is_a?(Float))
+    end
+
     def clean_host(host)
       strip_ipv6_brackets(host)
     end

spec/httparty/connection_adapter_spec.rb

@@ -131,6 +131,7 @@ RSpec.describe HTTParty::ConnectionAdapter do
           )
           expect(http).not_to receive(:open_timeout=)
           expect(http).not_to receive(:read_timeout=)
+          expect(http).not_to receive(:write_timeout=)
           allow(Net::HTTP).to receive_messages(new: http)
 
           adapter.connection
@@ -150,6 +151,13 @@ RSpec.describe HTTParty::ConnectionAdapter do
             subject { super().read_timeout }
             it { is_expected.to eq(5) }
           end
+
+          if RUBY_VERSION >= '2.6.0'
+            describe '#write_timeout' do
+              subject { super().write_timeout }
+              it { is_expected.to eq(5) }
+            end
+          end
         end
 
         context "and timeout is a string" do
@@ -164,6 +172,7 @@ RSpec.describe HTTParty::ConnectionAdapter do
             )
             expect(http).not_to receive(:open_timeout=)
             expect(http).not_to receive(:read_timeout=)
+            expect(http).not_to receive(:write_timeout=)
             allow(Net::HTTP).to receive_messages(new: http)
 
             adapter.connection
@@ -191,6 +200,19 @@ RSpec.describe HTTParty::ConnectionAdapter do
           allow(Net::HTTP).to receive_messages(new: http)
           adapter.connection
         end
+
+        it "should not set the write_timeout" do
+          http = double(
+            "http",
+            :null_object => true,
+            :use_ssl= => false,
+            :use_ssl? => false,
+            :read_timeout= => 0
+          )
+          expect(http).not_to receive(:write_timeout=)
+          allow(Net::HTTP).to receive_messages(new: http)
+          adapter.connection
+        end
       end
 
       context "when timeout is set and read_timeout is set to 6 seconds" do
@@ -201,6 +223,13 @@ RSpec.describe HTTParty::ConnectionAdapter do
           it { is_expected.to eq(5) }
         end
 
+        if RUBY_VERSION >= '2.6.0'
+          describe '#write_timeout' do
+            subject { super().write_timeout }
+            it { is_expected.to eq(5) }
+          end
+        end
+
         describe '#read_timeout' do
           subject { super().read_timeout }
           it { is_expected.to eq(6) }
@@ -213,10 +242,14 @@ RSpec.describe HTTParty::ConnectionAdapter do
             :use_ssl= => false,
             :use_ssl? => false,
             :read_timeout= => 0,
-            :open_timeout= => 0
+            :open_timeout= => 0,
+            :write_timeout= => 0,
           )
           expect(http).to receive(:open_timeout=)
           expect(http).to receive(:read_timeout=).twice
+          if RUBY_VERSION >= '2.6.0'
+            expect(http).to receive(:write_timeout=)
+          end
           allow(Net::HTTP).to receive_messages(new: http)
           adapter.connection
         end
@@ -242,6 +275,19 @@ RSpec.describe HTTParty::ConnectionAdapter do
           allow(Net::HTTP).to receive_messages(new: http)
           adapter.connection
         end
+
+        it "should not set the write_timeout" do
+          http = double(
+            "http",
+            :null_object => true,
+            :use_ssl= => false,
+            :use_ssl? => false,
+            :open_timeout= => 0
+          )
+          expect(http).not_to receive(:write_timeout=)
+          allow(Net::HTTP).to receive_messages(new: http)
+          adapter.connection
+        end
       end
 
       context "when timeout is set and open_timeout is set to 7 seconds" do
@@ -252,6 +298,13 @@ RSpec.describe HTTParty::ConnectionAdapter do
           it { is_expected.to eq(7) }
         end
 
+        if RUBY_VERSION >= '2.6.0'
+          describe '#write_timeout' do
+            subject { super().write_timeout }
+            it { is_expected.to eq(5) }
+          end
+        end
+
         describe '#read_timeout' do
           subject { super().read_timeout }
           it { is_expected.to eq(5) }
@@ -264,15 +317,88 @@ RSpec.describe HTTParty::ConnectionAdapter do
             :use_ssl= => false,
             :use_ssl? => false,
             :read_timeout= => 0,
-            :open_timeout= => 0
+            :open_timeout= => 0,
+            :write_timeout= => 0,
           )
           expect(http).to receive(:open_timeout=).twice
           expect(http).to receive(:read_timeout=)
+          if RUBY_VERSION >= '2.6.0'
+            expect(http).to receive(:write_timeout=)
+          end
           allow(Net::HTTP).to receive_messages(new: http)
           adapter.connection
         end
       end
 
+      if RUBY_VERSION >= '2.6.0'
+        context "when timeout is not set and write_timeout is set to 8 seconds" do
+          let(:options) { {write_timeout: 8} }
+
+          describe '#write_timeout' do
+            subject { super().write_timeout }
+            it { is_expected.to eq(8) }
+          end
+
+          it "should not set the open timeout" do
+            http = double(
+              "http",
+              :null_object => true,
+              :use_ssl= => false,
+              :use_ssl? => false,
+              :read_timeout= => 0,
+              :open_timeout= => 0,
+              :write_timeout= => 0,
+
+            )
+            expect(http).not_to receive(:open_timeout=)
+            allow(Net::HTTP).to receive_messages(new: http)
+            adapter.connection
+          end
+
+          it "should not set the read timeout" do
+            http = double(
+              "http",
+              :null_object => true,
+              :use_ssl= => false,
+              :use_ssl? => false,
+              :read_timeout= => 0,
+              :open_timeout= => 0,
+              :write_timeout= => 0,
+
+            )
+            expect(http).not_to receive(:read_timeout=)
+            allow(Net::HTTP).to receive_messages(new: http)
+            adapter.connection
+          end
+        end
+
+        context "when timeout is set and write_timeout is set to 8 seconds" do
+          let(:options) { {timeout: 2, write_timeout: 8} }
+
+          describe '#write_timeout' do
+            subject { super().write_timeout }
+            it { is_expected.to eq(8) }
+          end
+
+          it "should override the timeout option" do
+            http = double(
+              "http",
+              :null_object => true,
+              :use_ssl= => false,
+              :use_ssl? => false,
+              :read_timeout= => 0,
+              :open_timeout= => 0,
+              :write_timeout= => 0,
+            )
+            expect(http).to receive(:read_timeout=)
+            expect(http).to receive(:open_timeout=)
+            expect(http).to receive(:write_timeout=).twice
+            allow(Net::HTTP).to receive_messages(new: http)
+            adapter.connection
+          end
+        end
+      end
+
       context "when debug_output" do
         let(:http) { Net::HTTP.new(uri) }
         before do

spec/httparty_spec.rb

@@ -329,6 +329,10 @@ RSpec.describe HTTParty do
       @klass.default_timeout 0.5
       expect(@klass.default_options[:timeout]).to eq(0.5)
     end
+
+    it "should raise an exception if unsupported type provided" do
+      expect { @klass.default_timeout "0.5" }.to raise_error ArgumentError
+    end
   end
 
   describe "debug_output" do