Project

General

Profile

Actions

Bug #5765

closed

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

Added by normalperson (Eric Wong) over 12 years ago. Updated about 12 years ago.

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

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


Files


Related issues 2 (0 open2 closed)

Related to Ruby master - Feature #6088: Add Net::ReadTimeout to distinguish which operation failedCloseddrbrain (Eric Hodel)02/26/2012Actions
Related to Ruby master - Bug #6001: Retry idempotent HTTP requests for more errorsClosed02/11/2012Actions

Updated by ko1 (Koichi Sasada) about 12 years ago

Any progress?

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

Updated by mame (Yusuke Endoh) about 12 years ago

2012/2/25 Koichi Sasada :

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

An enthusiastic +1.

--
Yusuke Endoh

Updated by normalperson (Eric Wong) about 12 years ago

Koichi Sasada 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 [ruby-core:38807],
I am not interested in being a committer.

Updated by drbrain (Eric Hodel) about 12 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.

Updated by drbrain (Eric Hodel) about 12 years ago

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

Updated by normalperson (Eric Wong) about 12 years ago

Eric Hodel 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.modernize_timeout_usage.open_timeout.patch looks
good to me. Thanks!

Actions #7

Updated by drbrain (Eric Hodel) about 12 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

Updated by drbrain (Eric Hodel) about 12 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.

Updated by normalperson (Eric Wong) about 12 years ago

Eric Hodel 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0