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

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

[normalperson (Eric Wong)]

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

[ko1 (Koichi Sasada)]

Any progress?

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

[mame (Yusuke Endoh)]

2012/2/25 Koichi Sasada [e-mail]:

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

An enthusiastic +1.

--
Yusuke Endoh [e-mail]

[normalperson (Eric Wong)]

Koichi Sasada [e-mail] 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.

[drbrain (Eric Hodel)]

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.

[drbrain (Eric Hodel)]

• File net.modernize_timeout_usage.open_timeout.patch net.modernize_timeout_usage.open_timeout.patch added

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

[normalperson (Eric Wong)]

Eric Hodel [e-mail] 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!

[drbrain (Eric Hodel)]

• 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

[drbrain (Eric Hodel)]

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.

[normalperson (Eric Wong)]

Eric Hodel [e-mail] 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.