Feature #7503

make timeout.rb async-interrupt safe by default

Added by Motohiro KOSAKI over 1 year ago. Updated over 1 year ago.

[ruby-core:50524]
Status:Assigned
Priority:Normal
Assignee:Yukihiro Matsumoto
Category:core
Target version:next minor

Description

Hi

Again and again we discussed, current timeout.rb is very dangerous because ExitException interrupt argument code and unwind call stack immediately.
It may prevent to run ensure block and makes resource leak.

I proposed change default to interrupted only on blocking point.

patch is here.
https://gist.github.com/4195015

I also propse to add 'immediate' optional argument because it may help to make a workaround timeout.rb + zero blocking point corner case.

What do you think?

Associated revisions

Revision 38216
Added by Motohiro KOSAKI over 1 year ago

  • lib/timeout.rb (Timeout#timeout): set
    asyncinterrupttimeing(:on_blocking) by default.
    [Bug #7503]

  • test/testtimeout.rb (#testtimeout_blocking): test for the above.

  • test/testtimeout.rb (testtimeout_immediate): ditto

  • test/testtimeout.rb (testtimeout_immediate2): ditto.

  • NEWS: news for the above.

Revision 38255
Added by Nobuyoshi Nakada over 1 year ago

timeout.rb: replace deferred exception after asyncinterrupttiming

  • lib/timeout.rb (Timeout#timeout): since asyncinterrupttiming re-raises a deferred exception, replace the timeout exception with Timeout::Error after it. [Bug #7503]

History

#1 Updated by Koichi Sasada over 1 year ago

Actually, I like your proposal.
But I make weak objection.

The following code doesn't stop forever.

timeout(3){
# long calculation / infinite loop w/o IO operation
}

I guess most of case, the block includes IO operation and no problem.
However, there are several case for it.

This is Dec and Preview 2 was released.
I think it is too slow to introduce it.

Again, I like this proposal.
If no people have object, I can agree with this proposal.

Alternative proposal is making such a safe timeout method with different
name such as `safe_timeout'.

(2012/12/03 22:33), kosaki (Motohiro KOSAKI) wrote:

Issue #7503 has been reported by kosaki (Motohiro KOSAKI).


Bug #7503: make timeout.rb async-interrupt safe by default
https://bugs.ruby-lang.org/issues/7503

Author: kosaki (Motohiro KOSAKI)
Status: Open
Priority: Normal
Assignee:
Category: core
Target version: 2.0.0
ruby -v: trunk

Hi

Again and again we discussed, current timeout.rb is very dangerous because ExitException interrupt argument code and unwind call stack immediately.
It may prevent to run ensure block and makes resource leak.

I proposed change default to interrupted only on blocking point.

patch is here.
https://gist.github.com/4195015

I also propse to add 'immediate' optional argument because it may help to make a workaround timeout.rb + zero blocking point corner case.

What do you think?

--
// SASADA Koichi at atdot dot net

#2 Updated by Motohiro KOSAKI over 1 year ago

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

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


  • lib/timeout.rb (Timeout#timeout): set
    asyncinterrupttimeing(:on_blocking) by default.
    [Bug #7503]

  • test/testtimeout.rb (#testtimeout_blocking): test for the above.

  • test/testtimeout.rb (testtimeout_immediate): ditto

  • test/testtimeout.rb (testtimeout_immediate2): ditto.

  • NEWS: news for the above.

#3 Updated by Koichi Sasada over 1 year ago

  • Status changed from Closed to Open

I need to make strongly objection about this commit (r38216) because this discussion is not concluded.

I believe at least mame-san's permission is needed to introduce this change.
This is big behavior change.

#4 Updated by Koichi Sasada over 1 year ago

  • Assignee set to Yusuke Endoh
  • Priority changed from Normal to High

#5 Updated by Yusuke Endoh over 1 year ago

  • Tracker changed from Bug to Feature
  • Status changed from Open to Assigned
  • Assignee changed from Yusuke Endoh to Yukihiro Matsumoto

I do NOT accept r38216 for 2.0.0. Please revert it.

Not only it is a new feature missing the deadline, but also it will lead
to actual compatibility issues. I often use timeout to bound not only IO
operation but also pure calculation, such as game tree search and brute-
force search.

(Thanks ko1 for letting me know!)

Yusuke Endoh mame@tsg.ne.jp

#6 Updated by Yusuke Endoh over 1 year ago

  • Priority changed from High to Normal
  • Target version changed from 2.0.0 to next minor

#7 Updated by Nobuyoshi Nakada over 1 year ago

  • % Done changed from 100 to 50

#8 Updated by Nobuyoshi Nakada over 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 50 to 100

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


timeout.rb: replace deferred exception after asyncinterrupttiming

  • lib/timeout.rb (Timeout#timeout): since asyncinterrupttiming re-raises a deferred exception, replace the timeout exception with Timeout::Error after it. [Bug #7503]

#9 Updated by Motohiro KOSAKI over 1 year ago

  • Status changed from Closed to Assigned

Target version changed from 2.0.0 to next minor

r38216 was reverted then.

Also available in: Atom PDF