Bug #6001: Retry idempotent HTTP requests for more errors

• Status: Closed
• Assignee: -
• Target version: 2.0.0
• ruby -v: ruby 2.0.0dev (2012-02-11 trunk 34547) [x86_64-darwin11.2.0]

[drbrain (Eric Hodel)]

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.

[drbrain (Eric Hodel)]

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

[drbrain (Eric Hodel)]

• File net.http.retry_errors.patch net.http.retry_errors.patch added

[naruse (Yui NARUSE)]

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.

[drbrain (Eric Hodel)]

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.

[drbrain (Eric Hodel)]

• File net.http.retry_errors.2.patch net.http.retry_errors.2.patch added

=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

[naruse (Yui NARUSE)]

•  assert_raises(EOFError) {
  

•  assert_raises(IOError) {
  

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

Others are ok.

[drbrain (Eric Hodel)]

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

[naruse (Yui NARUSE)]

Eric Hodel wrote:

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

Yes, please

[drbrain (Eric Hodel)]

• 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