Bug #5765

[PATCH] modernize Timeout usage in net/{http,pop,smtp,telnet}

Added by Eric Wong over 2 years ago. Updated about 2 years ago.

[ruby-core:41662]
Status:Closed
Priority:Low
Assignee:-
Category:lib
Target version:2.0.0
ruby -v:ruby 2.0.0dev (2011-12-14 trunk 34045) [x86_64-linux] Backport:

Description

Object#timeout is deprecated, so use Timeout.timeout instead

Additionally, rely on raising Timeout::Error instead of
temporary Class object. Using a temporary Class object only
benefits nested timeouts, but the code paths in net/* do not
nest timeouts.

Unfortunately, any performance benefit of avoiding temporary
Class objects is nullified here because Errno::EAGAIN is
extended with IO::WaitReadable or IO::WaitWritable:
https://bugs.ruby-lang.org/issues/5138

A better version of this patch would use timeouts via select()
when handling DNS lookup and non-blocking connect(). That
requires more effort to deal with hostnames that resolve to
multiple addresses, so we'll revisit that later.

This change is also pullable:

git pull git://bogomips.org/ruby modern-timeout

0001-modernize-Timeout-usage-in-net-http-pop-smtp-telnet.patch Magnifier (3.65 KB) Eric Wong, 12/15/2011 10:11 AM

net.modernize_timeout_usage.open_timeout.patch Magnifier (4.56 KB) Eric Hodel, 02/26/2012 07:22 AM


Related issues

Related to ruby-trunk - Feature #6088: Add Net::ReadTimeout to distinguish which operation failed Closed 02/26/2012
Related to ruby-trunk - Bug #6001: Retry idempotent HTTP requests for more errors Closed 02/11/2012

Associated revisions

Revision 34843
Added by Eric Hodel about 2 years ago

  • lib/net/protocol.rb: Add OpenTimeout subclass of Timeout::Error
    • lib/net/pop.rb: Modernize Timeout usage. Patch by Eric Wong. Use Net::OpenTimeout instead of Timeout::Error. [Bug #5765]
    • lib/net/http.rb: ditto
    • lib/net/smtp.rb: ditto
    • lib/net/telnet.rb: ditto

History

#1 Updated by Koichi Sasada about 2 years ago

Any progress?

I think it is good timing that you become a committer :)
(I'm sorry if you already are)

#2 Updated by Yusuke Endoh about 2 years ago

2012/2/25 Koichi Sasada redmine@ruby-lang.org:

I think it is good timing that you become a committer :)
(I'm sorry if you already are)

An enthusiastic +1.

--
Yusuke Endoh mame@tsg.ne.jp

#3 Updated by Eric Wong about 2 years ago

Koichi Sasada redmine@ruby-lang.org wrote:

Any progress?

Patch still applies to trunk with offsets. I was hoping somebody
else can review it (I never trust anything I write :)

I think it is good timing that you become a committer :)
(I'm sorry if you already are)

Thank you for the thought, but as with ,
I am not interested in being a committer.

#4 Updated by Eric Hodel about 2 years ago

I like this patch.

This patch will collide with #6001, though, since I introduce Net::HTTP::OpenTimeout as a subclass of Timeout::Error. Should we add Net::OpenTimeout instead of Net::HTTP::OpenTimeout?

See also #6088 where I introduce Net::ReadTimeout. The division between Net::ReadTimeout and Net::OpenTimeout will allow users to distinguish which operation timed out. For a read timeout they may wish to retry the operation.

#5 Updated by Eric Hodel about 2 years ago

The attached patch combines the original patch with the introduction of Net::OpenTimeout.

#6 Updated by Eric Wong about 2 years ago

Eric Hodel drbrain@segment7.net wrote:

I like this patch.

This patch will collide with #6001, though, since I introduce
Net::HTTP::OpenTimeout as a subclass of Timeout::Error. Should we add
Net::OpenTimeout instead of Net::HTTP::OpenTimeout?

Yes, I vastly prefer Net::OpenTimeout since timeouts shouldn't be
limited to HTTP.

See also #6088 where I introduce Net::ReadTimeout. The division
between Net::ReadTimeout and Net::OpenTimeout will allow users to
distinguish which operation timed out. For a read timeout they may
wish to retry the operation.

Excellent. Your net.modernizetimeoutusage.open_timeout.patch looks
good to me. Thanks!

#7 Updated by Eric Hodel about 2 years ago

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

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


  • lib/net/protocol.rb: Add OpenTimeout subclass of Timeout::Error
    • lib/net/pop.rb: Modernize Timeout usage. Patch by Eric Wong. Use Net::OpenTimeout instead of Timeout::Error. [Bug #5765]
    • lib/net/http.rb: ditto
    • lib/net/smtp.rb: ditto
    • lib/net/telnet.rb: ditto

#8 Updated by Eric Hodel about 2 years ago

Since the original patch was approved and Net::HTTP::OpenTimeout was approved in #6001, I applied a combined patch in r34843, replacing Net::HTTP::OpenTimeout.

#9 Updated by Eric Wong about 2 years ago

Eric Hodel drbrain@segment7.net wrote:

Since the original patch was approved and Net::HTTP::OpenTimeout was
approved in #6001, I applied a combined patch in r34843, replacing
Net::HTTP::OpenTimeout.

Thanks! I hope to make Timeout itself better/less-racy in the future.

Also available in: Atom PDF