Bug #6001

Retry idempotent HTTP requests for more errors

Added by Eric Hodel over 3 years ago. Updated about 3 years ago.

[ruby-core:42495]
Status:Closed
Priority:Normal
Assignee:-
ruby -v:ruby 2.0.0dev (2012-02-11 trunk 34547) [x86_64-darwin11.2.0] Backport:

Description

The net-http-persistent gem implements #5790 / #5813 but for more error types:

https://github.com/drbrain/net-http-persistent/blob/1b5f84d020/lib/net/http/persistent.rb#L811-813

These additional error types should be rescued by net/http and retried.

net.http.retry_errors.patch Magnifier - Oops, fixed patch. (661 Bytes) Eric Hodel, 02/11/2012 09:57 AM

net.http.retry_errors.2.patch Magnifier - Improved patch - separates open from read errors (3.66 KB) Eric Hodel, 02/25/2012 05:04 AM


Related issues

Related to Ruby trunk - Bug #5790: net/http の EOFError と Keep-Alive Closed 12/22/2011
Related to Ruby trunk - Bug #5813: net/http's EOFError and Keep-Alive Closed 12/27/2011
Related to Ruby trunk - Bug #5765: [PATCH] modernize Timeout usage in net/{http,pop,smtp,telnet} Closed 12/15/2011

Associated revisions

Revision 34842
Added by Eric Hodel about 3 years ago

  • lib/net/http.rb: Retry HTTP requests for additional network errors. Introduce OpenTimeout subclass of Timeout::Error. [Bug #6001]
    • test/net/http/test_http.rb: Reduce timeout to 0.01s for faster test
    • test/net/http/test_https.rb: ditto

Revision 34842
Added by Eric Hodel about 3 years ago

  • lib/net/http.rb: Retry HTTP requests for additional network errors. Introduce OpenTimeout subclass of Timeout::Error. [Bug #6001]
    • test/net/http/test_http.rb: Reduce timeout to 0.01s for faster test
    • test/net/http/test_https.rb: ditto

History

#1 Updated by Eric Hodel over 3 years ago

  • File deleted (net.http.retry_errors.patch)

#3 Updated by Yui NARUSE over 3 years ago

I agree the general direction to add more exceptions like ECONNABORTED.
But I doubt Timeout::Error and should be added.

A general principle is to rescue temporally errors.
I don't think Errno::EINVAL, OpenSSL::SSL::SSLError are such errors.

anyway, the patch includes Errno::ECONNRESET twice.

#4 Updated by Eric Hodel over 3 years ago

I think Timeout::Error is OK since Errno::ECONNRESET or Errno::ECONNABORTED may race with your timeout setting depending upon network behavior.

OpenSSL::SSL::SSLError may be raised due to a temporary issue when reading or writing and it is not possible to distinguish between temporary and permanent SSLErrors, so I think retrying is OK.

I agree that EINVAL should be removed.

#5 Updated by Eric Hodel over 3 years ago

=begin
Please check this improved patch.

The new patch separates open and connect errors from response read/write errors. This allows OpenSSL::SSL::SSLError to be rescued only when there is an error in the connection (from SSL_read or SSL_write) and not an error in the certificate verification or other permanent SSL issues.

The new patch introduces Net::HTTP::OpenTimeout to separate a timeout for opening a connection from response read/write timeouts.

The new patch ignores Errno::EINVAL and removes the duplicated Errno::ECONNRESET
=end

#6 Updated by Yui NARUSE over 3 years ago

  • assert_raises(EOFError) {
  • assert_raises(IOError) {

On my environment, EOFError and Errno::ECONNRESET may happen.

Others are ok.

#7 Updated by Eric Hodel over 3 years ago

=begin
If I update to (({assert_raises(EOFError, IOError, Errno::ECONNRESET) {})) may I commit?
=end

#8 Updated by Yui NARUSE over 3 years ago

Eric Hodel wrote:

If I update to (({assert_raises(EOFError, IOError, Errno::ECONNRESET) {})) may I commit?

Yes, please

#9 Updated by Eric Hodel about 3 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r34842.
Eric, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • lib/net/http.rb: Retry HTTP requests for additional network errors. Introduce OpenTimeout subclass of Timeout::Error. [Bug #6001]
    • test/net/http/test_http.rb: Reduce timeout to 0.01s for faster test
    • test/net/http/test_https.rb: ditto

Also available in: Atom PDF