diff --git a/History.txt b/History.txt index 7cff890..d059b06 100644 --- a/History.txt +++ b/History.txt @@ -1,3 +1,9 @@ +=== Next + +Breaking changes: + +* Requests are no longer retried by net-http-persistent. + === 3.0 Breaking changes: diff --git a/lib/net/http/persistent.rb b/lib/net/http/persistent.rb index 2f5ebc8..adc6cc8 100644 --- a/lib/net/http/persistent.rb +++ b/lib/net/http/persistent.rb @@ -136,45 +136,6 @@ # Socket options may be set on newly-created connections. See #socket_options # for details. # -# === Non-Idempotent Requests -# -# By default non-idempotent requests will not be retried per RFC 2616. By -# setting retry_change_requests to true requests will automatically be retried -# once. -# -# Only do this when you know that retrying a POST or other non-idempotent -# request is safe for your application and will not create duplicate -# resources. -# -# The recommended way to handle non-idempotent requests is the following: -# -# require 'net/http/persistent' -# -# uri = URI 'http://example.com/awesome/web/service' -# post_uri = uri + 'create' -# -# http = Net::HTTP::Persistent.new name: 'my_app_name' -# -# post = Net::HTTP::Post.new post_uri.path -# # ... fill in POST request -# -# begin -# response = http.request post_uri, post -# rescue Net::HTTP::Persistent::Error -# -# # POST failed, make a new request to verify the server did not process -# # the request -# exists_uri = uri + '...' -# response = http.get exists_uri -# -# # Retry if it failed -# retry if response.code == '404' -# end -# -# The method of determining if the resource was created or not is unique to -# the particular service you are using. Of course, you will want to add -# protection from infinite looping. -# # === Connection Termination # # If you are done using the Net::HTTP::Persistent instance you may shut down @@ -209,21 +170,6 @@ class Net::HTTP::Persistent VERSION = '3.0.0' - ## - # Exceptions rescued for automatic retry on ruby 2.0.0. This overlaps with - # the exception list for ruby 1.x. - - RETRIED_EXCEPTIONS = [ # :nodoc: - (Net::ReadTimeout if Net.const_defined? :ReadTimeout), - IOError, - EOFError, - Errno::ECONNRESET, - Errno::ECONNABORTED, - Errno::EPIPE, - (OpenSSL::SSL::SSLError if HAVE_OPENSSL), - Timeout::Error, - ].compact - ## # Error class for errors raised by Net::HTTP::Persistent. Various # SystemCallErrors are re-raised with a human-readable message under this @@ -491,17 +437,6 @@ def self.detect_idle_timeout uri, max = 10 attr_reader :verify_mode - ## - # Enable retries of non-idempotent requests that change data (e.g. POST - # requests) when the server has disconnected. - # - # This will in the worst case lead to multiple requests with the same data, - # but it may be useful for some applications. Take care when enabling - # this option to ensure it is safe to POST or perform other non-idempotent - # requests to the server. - - attr_accessor :retry_change_requests - ## # Creates a new Net::HTTP::Persistent. # @@ -569,8 +504,6 @@ def initialize name: nil, proxy: nil, pool_size: DEFAULT_POOL_SIZE @reuse_ssl_sessions = OpenSSL::SSL.const_defined? :Session end - @retry_change_requests = false - self.proxy = proxy if proxy end @@ -667,19 +600,6 @@ def connection_for uri @pool.checkin net_http_args end - ## - # Returns an error message containing the number of requests performed on - # this connection - - def error_message connection - connection.requests -= 1 # fixup - - age = Time.now - connection.last_use - - "after #{connection.requests} requests on #{connection.http.object_id}, " \ - "last used #{age} seconds ago" - end - ## # URI::escape wrapper @@ -742,24 +662,6 @@ def http_version uri @http_versions["#{uri.host}:#{uri.port}"] end - ## - # Is +req+ idempotent according to RFC 2616? - - def idempotent? req - case req - when Net::HTTP::Delete, Net::HTTP::Get, Net::HTTP::Head, - Net::HTTP::Options, Net::HTTP::Put, Net::HTTP::Trace then - true - end - end - - ## - # Is the request +req+ idempotent or is retry_change_requests allowed. - - def can_retry? req - @retry_change_requests && !idempotent?(req) - end - ## # Adds "http://" to the String +uri+ if it is missing. @@ -937,14 +839,8 @@ def reset connection # the response will not have been read). # # +req+ must be a Net::HTTPRequest subclass (see Net::HTTP for a list). - # - # If there is an error and the request is idempotent according to RFC 2616 - # it will be retried automatically. def request uri, req = nil, &block - retried = false - bad_response = false - uri = URI uri req = request_setup req || uri response = nil @@ -958,37 +854,12 @@ def request uri, req = nil, &block response = http.request req, &block if req.connection_close? or - (response.http_version <= '1.0' and + (response.http_version <= '1.0' and not response.connection_keep_alive?) or - response.connection_close? then + response.connection_close? then finish connection end - rescue Net::HTTPBadResponse => e - message = error_message connection - - finish connection - - raise Error, "too many bad responses #{message}" if - bad_response or not can_retry? req - - bad_response = true - retry - rescue *RETRIED_EXCEPTIONS => e - request_failed e, req, connection if - retried or not can_retry? req - - reset connection - - retried = true - retry - rescue Errno::EINVAL, Errno::ETIMEDOUT => e # not retried on ruby 2 - request_failed e, req, connection if retried or not can_retry? req - - reset connection - - retried = true - retry - rescue Exception => e + rescue Exception # make sure to close the connection when it was interrupted finish connection raise @@ -1002,21 +873,6 @@ def request uri, req = nil, &block response end - ## - # Raises an Error for +exception+ which resulted from attempting the request - # +req+ on the +connection+. - # - # Finishes the +connection+. - - def request_failed exception, req, connection # :nodoc: - due_to = "(due to #{exception.message} - #{exception.class})" - message = "too many connection resets #{due_to} #{error_message connection}" - - finish connection - - raise Error, message, exception.backtrace - end - ## # Creates a GET request if +req_or_uri+ is a URI and adds headers to the # request. @@ -1188,7 +1044,6 @@ def verify_callback= callback reconnect_ssl end - end require 'net/http/persistent/connection' diff --git a/test/test_net_http_persistent.rb b/test/test_net_http_persistent.rb index dfe3ef8..5aefedb 100644 --- a/test/test_net_http_persistent.rb +++ b/test/test_net_http_persistent.rb @@ -220,26 +220,6 @@ def test_ca_path_equals assert_equal 1, @http.ssl_generation end - def test_can_retry_eh_change_requests - post = Net::HTTP::Post.new '/' - - refute @http.can_retry? post - - @http.retry_change_requests = true - - assert @http.can_retry? post - end - - def test_can_retry_eh_idempotent - head = Net::HTTP::Head.new '/' - - refute @http.can_retry? head - - post = Net::HTTP::Post.new '/' - - refute @http.can_retry? post - end - def test_cert_store_equals @http.cert_store = :cert_store @@ -612,16 +592,6 @@ def test_connection_for_timeout end end - def test_error_message - c = basic_connection - c.last_use = Time.now - 1 - c.requests = 5 - - message = @http.error_message c - assert_match %r%after 4 requests on #{c.http.object_id}%, message - assert_match %r%, last used [\d.]+ seconds ago%, message - end - def test_escape assert_nil @http.escape nil @@ -720,17 +690,6 @@ def test_http_version assert_equal '1.1', @http.http_version(@uri) end - def test_idempotent_eh - assert @http.idempotent? Net::HTTP::Delete.new '/' - assert @http.idempotent? Net::HTTP::Get.new '/' - assert @http.idempotent? Net::HTTP::Head.new '/' - assert @http.idempotent? Net::HTTP::Options.new '/' - assert @http.idempotent? Net::HTTP::Put.new '/' - assert @http.idempotent? Net::HTTP::Trace.new '/' - - refute @http.idempotent? Net::HTTP::Post.new '/' - end - def test_normalize_uri assert_equal 'http://example', @http.normalize_uri('example') assert_equal 'http://example', @http.normalize_uri('http://example') @@ -945,7 +904,7 @@ def (ssl_http.http).finish assert_equal Net::HTTP::Persistent::EPOCH, used2.last_use end - def test_request + def test_requestx @http.override_headers['user-agent'] = 'test ua' @http.headers['accept'] = 'text/*' c = connection @@ -969,62 +928,6 @@ def test_request assert_equal 1, c.requests end - def test_request_ETIMEDOUT - c = basic_connection - def (c.http).request(*a) raise Errno::ETIMEDOUT, "timed out" end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request @uri - end - - assert_equal 0, c.requests - assert_match %r%too many connection resets%, e.message - end - - def test_request_bad_response - c = basic_connection - def (c.http).request(*a) raise Net::HTTPBadResponse end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request @uri - end - - assert_equal 0, c.requests - assert_match %r%too many bad responses%, e.message - end - - def test_request_bad_response_retry - c = basic_connection - def (c.http).request(*a) - raise Net::HTTPBadResponse - end - - assert_raises Net::HTTP::Persistent::Error do - @http.request @uri - end - - assert c.http.finished? - end - - def test_request_bad_response_unsafe - c = basic_connection - def (c.http).request(*a) - if instance_variable_defined? :@request then - raise 'POST must not be retried' - else - @request = true - raise Net::HTTPBadResponse - end - end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request @uri, Net::HTTP::Post.new(@uri.path) - end - - assert_equal 0, c.requests - assert_match %r%too many bad responses%, e.message - end - def test_request_block @http.headers['user-agent'] = 'test ua' c = connection @@ -1150,12 +1053,12 @@ def test_request_invalid c = basic_connection def (c.http).request(*a) raise Errno::EINVAL, "write" end - e = assert_raises Net::HTTP::Persistent::Error do + e = assert_raises Errno::EINVAL do @http.request @uri end assert_equal 0, c.requests - assert_match %r%too many connection resets%, e.message + assert_match %r%Invalid argument - write%, e.message end def test_request_post @@ -1169,69 +1072,6 @@ def test_request_post assert_same post, req end - def test_request_reset - c = basic_connection - def (c.http).request(*a) raise Errno::ECONNRESET end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request @uri - end - - assert_equal 0, c.requests - assert_match %r%too many connection resets%, e.message - end - - def test_request_reset_retry - c = basic_connection - c.last_use = Time.now - - def (c.http).request(*a) - raise Errno::ECONNRESET - end - - assert_raises Net::HTTP::Persistent::Error do - @http.request @uri - end - - refute (c.http).reset? - assert (c.http).finished? - end - - def test_request_reset_unsafe - c = basic_connection - def (c.http).request(*a) - if instance_variable_defined? :@request then - raise 'POST must not be retried' - else - @request = true - raise Errno::ECONNRESET - end - end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request @uri, Net::HTTP::Post.new(@uri.path) - end - - assert_equal 0, c.requests - assert_match %r%too many connection resets%, e.message - end - - def test_request_ssl_error - skip 'OpenSSL is missing' unless HAVE_OPENSSL - - uri = URI.parse 'https://example.com/path' - @http.connection_for uri do |c| - def (c.http).request(*) - raise OpenSSL::SSL::SSLError, "SSL3_WRITE_PENDING:bad write retry" - end - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request uri - end - assert_match %r%bad write retry%, e.message - end - end - def test_request_setup @http.override_headers['user-agent'] = 'test ua' @http.headers['accept'] = 'text/*' @@ -1275,30 +1115,6 @@ def test_request_setup_uri assert_equal '/path?a=b', req.path end - def test_request_failed - c = basic_connection - c.requests = 1 - c.last_use = Time.now - - original = nil - - begin - raise 'original' - rescue => original - end - - req = Net::HTTP::Get.new '/' - - e = assert_raises Net::HTTP::Persistent::Error do - @http.request_failed original, req, c - end - - assert_match "too many connection resets (due to original - RuntimeError)", - e.message - - assert_equal original.backtrace, e.backtrace - end - def test_reset c = basic_connection c.http.start @@ -1353,23 +1169,6 @@ def (c.http).start; raise Errno::ECONNREFUSED end assert_match __FILE__, e.backtrace.first end - def test_retry_change_requests_equals - get = Net::HTTP::Get.new('/') - post = Net::HTTP::Post.new('/') - - refute @http.retry_change_requests - - refute @http.can_retry?(get) - refute @http.can_retry?(post) - - @http.retry_change_requests = true - - assert @http.retry_change_requests - - refute @http.can_retry?(get) - assert @http.can_retry?(post) - end - def test_shutdown c = connection