Feature #7503

make timeout.rb async-interrupt safe by default

Added by Motohiro KOSAKI over 2 years ago. Updated over 2 years ago.

[ruby-core:50524]
Status:Assigned
Priority:Normal
Assignee:Yukihiro Matsumoto

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 2 years ago

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

  • test/test_timeout.rb (#test_timeout_blocking): test for the above.

  • test/test_timeout.rb (test_timeout_immediate): ditto

  • test/test_timeout.rb (test_timeout_immediate2): ditto.

  • NEWS: news for the above.

Revision 38216
Added by Motohiro KOSAKI over 2 years ago

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

  • test/test_timeout.rb (#test_timeout_blocking): test for the above.

  • test/test_timeout.rb (test_timeout_immediate): ditto

  • test/test_timeout.rb (test_timeout_immediate2): ditto.

  • NEWS: news for the above.

Revision 38255
Added by Nobuyoshi Nakada over 2 years ago

timeout.rb: replace deferred exception after async_interrupt_timing

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

Revision 38255
Added by Nobuyoshi Nakada over 2 years ago

timeout.rb: replace deferred exception after async_interrupt_timing

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

History

#1 Updated by Koichi Sasada over 2 years 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 2 years 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
    async_interrupt_timeing(:on_blocking) by default.
    [Bug #7503]

  • test/test_timeout.rb (#test_timeout_blocking): test for the above.

  • test/test_timeout.rb (test_timeout_immediate): ditto

  • test/test_timeout.rb (test_timeout_immediate2): ditto.

  • NEWS: news for the above.

#3 Updated by Koichi Sasada over 2 years 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 2 years ago

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

#5 Updated by Yusuke Endoh over 2 years ago

  • Status changed from Open to Assigned
  • Tracker changed from Bug to Feature
  • 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 2 years ago

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

#7 Updated by Nobuyoshi Nakada over 2 years ago

  • % Done changed from 100 to 50

#8 Updated by Nobuyoshi Nakada over 2 years ago

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

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 async_interrupt_timing

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

#9 Updated by Motohiro KOSAKI over 2 years 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