Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise exception instead of throw/catch for timeouts #30

Merged
merged 5 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 15 additions & 19 deletions lib/timeout.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,24 @@
module Timeout
VERSION = "0.3.2"

# Internal error raised to when a timeout is triggered.
class ExitException < Exception
def exception(*)
self
end
end

# Raised by Timeout.timeout when the block times out.
class Error < RuntimeError
attr_reader :thread
def self.handle_timeout(message)
exc = ExitException.new(message)

def self.catch(*args)
exc = new(*args)
exc.instance_variable_set(:@thread, Thread.current)
exc.instance_variable_set(:@catch_value, exc)
::Kernel.catch(exc) {yield exc}
end

def exception(*)
# TODO: use Fiber.current to see if self can be thrown
if self.thread == Thread.current
bt = caller
begin
throw(@catch_value, bt)
rescue UncaughtThrowError
end
begin
yield exc
rescue ExitException => e
raise new(message) if exc.equal?(e)
raise
end
super
end
end

Expand Down Expand Up @@ -195,8 +192,7 @@ def timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+
if klass
perform.call(klass)
else
backtrace = Error.catch(&perform)
raise Error, message, backtrace
Error.handle_timeout(message, &perform)
end
end
module_function :timeout
Expand Down
40 changes: 33 additions & 7 deletions test/test_timeout.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,51 @@ def test_timeout
end
end

def test_nested_timeout
a = nil
assert_raise(Timeout::Error) do
Timeout.timeout(0.1) {
Timeout.timeout(1) {
nil while true
}
a = 1
}
end
assert_nil a
end

def test_cannot_convert_into_time_interval
bug3168 = '[ruby-dev:41010]'
def (n = Object.new).zero?; false; end
assert_raise(TypeError, bug3168) {Timeout.timeout(n) { sleep 0.1 }}
end

def test_skip_rescue
bug8730 = '[Bug #8730]'
def test_skip_rescue_standarderror
e = nil
assert_raise_with_message(Timeout::Error, /execution expired/, bug8730) do
assert_raise_with_message(Timeout::Error, /execution expired/) do
Timeout.timeout 0.01 do
begin
sleep 3
rescue Exception => e
rescue
jeremyevans marked this conversation as resolved.
Show resolved Hide resolved
flunk "should not see any exception but saw #{e.inspect}"
end
end
end
assert_nil(e, bug8730)
end

def test_raises_exception_internally
e = nil
assert_raise_with_message(Timeout::Error, /execution expired/) do
Timeout.timeout 0.01 do
begin
sleep 3
rescue Exception => exc
e = exc
raise
end
end
end
assert_equal Timeout::ExitException, e.class
end

def test_rescue_exit
Expand Down Expand Up @@ -127,11 +153,11 @@ def test_handle_interrupt
bug11344 = '[ruby-dev:49179] [Bug #11344]'
ok = false
assert_raise(Timeout::Error) {
Thread.handle_interrupt(Timeout::Error => :never) {
Thread.handle_interrupt(Timeout::ExitException => :never) {
Timeout.timeout(0.01) {
sleep 0.2
ok = true
Thread.handle_interrupt(Timeout::Error => :on_blocking) {
Thread.handle_interrupt(Timeout::ExitException => :on_blocking) {
Comment on lines +156 to +160
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to add a new public interface Timeout::ExitException.
Does it need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does if we want to allow users to use Thread.handle_interrupt with it. From @eregon's research, there are only 2 gems actually doing that, and whether it is actually needed is not known. If we decide we don't want to support that, we could make ExitException a private constant.

sleep 0.2
}
}
Expand Down