Bug #15886: return in rescue block breaks Timeout.timeout

• Status: Rejected
• Assignee: -
• Target version: -
• ruby -v: ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux-gnu]
• Backport: 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN

[moio (Silvio Moioli)]

Passing Timeout.timeout a block with a rescue clause that contains a return statement prevents Timeout::Error to be raised as expected.

Reproducer:

require 'timeout'

begin
  Timeout.timeout(1) do
    begin
      sleep 10
    ensure
      puts "ensure block executed"

      ## commenting line below restores expected behaviour
      return true

    end
  end
rescue Timeout::Error => e
  puts "EXPECTED BEHAVIOUR: timeout error rescued"
end

Expected output:

ensure block executed
EXPECTED BEHAVIOR: timeout error rescued

Actual output:

ensure block executed

Looking in Redmine the following two issues appear related (but I lack the insight to tell for sure):

• https://bugs.ruby-lang.org/issues/14859
• https://bugs.ruby-lang.org/issues/7503

I apologize in advance if this issue is a duplicate.

[jeremyevans0 (Jeremy Evans)]

• Status changed from Open to Rejected

I don't believe this is a bug or specifically related to Timeout (or to the other issues you linked). If an exception is raised and an ensure or a rescue block does an explicit return (or a non-local exit such as throw), the exception is ignored:

def a
  yield
ensure
  return :x
end

a { raise }
# => :x

Ruby programmers should be careful about use of explicit returns or non-local exits in rescue/ensure blocks.

[moio (Silvio Moioli)]

Well, as a Timeout.timeout user I would like to be able to wrap any code in a block and have a guarantee it will actually terminate after the specified number of seconds - no matter how the wrapped code looks like and especially if it is from a third party.

IIUC this is currently impossible to obtain fully with Timeout.timeout - certain code patterns like the one described will just break the above "contract". This is due to the combination of 1) the design of the Ruby language itself and 2) the implementation details of the method itself (specifically, the fact it internally raises an Exception as the means of blocking execution of the block, and that Exception is subject to the same semantics of any other Exception in Ruby, as you explained, particularly about rescue/ensure and return).

Can you confirm my understanding above is correct?

[jeremyevans0 (Jeremy Evans)]

moio (Silvio Moioli) wrote:

Well, as a Timeout.timeout user I would like to be able to wrap any code in a block and have a guarantee it will actually terminate after the specified number of seconds - no matter how the wrapped code looks like and especially if it is from a third party.

I don't think this is something Ruby can guarantee. If you really want that level of control, you need to run the code in a separate process, and kill it with operating system signals after the timeout (including SIGKILL if the process doesn't respond to other signals). Using a separate process means that you need to use some form of IPC to transfer data between the processes.

[moio (Silvio Moioli)]

jeremyevans0 (Jeremy Evans) wrote:

moio (Silvio Moioli) wrote:

Well, as a Timeout.timeout user I would like to be able to wrap any code in a block and have a guarantee it will actually terminate after the specified number of seconds - no matter how the wrapped code looks like and especially if it is from a third party.

I don't think this is something Ruby can guarantee. If you really want that level of control, you need to run the code in a separate process, [...]

I understand your reasoning - but in this case I would suggest to clarify the method documentation to be more explicit about the limits of what Ruby guarantees here, or equivalently stating the assumptions on the block for the method to behave as documented. If appropriate, a note about implementation details might also guide the programmer to decide the applicability to his/her own use case - at least to me this was not clear at all.

At the moment all https://docs.ruby-lang.org/en/2.6.0/Timeout.html#method-c-timeout states is "Perform an operation in a block, raising an error if it takes longer than sec seconds to complete", which at least to me naively meant what I stated above.

Would it make possible to at least treat this as a "documentation bug"?

[jeremyevans0 (Jeremy Evans)]

moio (Silvio Moioli) wrote:

jeremyevans0 (Jeremy Evans) wrote:

moio (Silvio Moioli) wrote:

Well, as a Timeout.timeout user I would like to be able to wrap any code in a block and have a guarantee it will actually terminate after the specified number of seconds - no matter how the wrapped code looks like and especially if it is from a third party.

I don't think this is something Ruby can guarantee. If you really want that level of control, you need to run the code in a separate process, [...]

I understand your reasoning - but in this case I would suggest to clarify the method documentation to be more explicit about the limits of what Ruby guarantees here, or equivalently stating the assumptions on the block for the method to behave as documented. If appropriate, a note about implementation details might also guide the programmer to decide the applicability to his/her own use case - at least to me this was not clear at all.

At the moment all https://docs.ruby-lang.org/en/2.6.0/Timeout.html#method-c-timeout states is "Perform an operation in a block, raising an error if it takes longer than sec seconds to complete", which at least to me naively meant what I stated above.

Would it make possible to at least treat this as a "documentation bug"?

The documentation is technically accurate, so I would not classify this as a documentation bug. The exception is raised, but the code swallows it.

However, I think it makes sense to expand the Timeout.timeout documentation to mention that you should not use it with blocks that you do not trust, with examples of things that can go wrong (such as your case).

[jeremyevans0 (Jeremy Evans)]

jeremyevans0 (Jeremy Evans) wrote:

However, I think it makes sense to expand the Timeout.timeout documentation to mention that you should not use it with blocks that you do not trust, with examples of things that can go wrong (such as your case).

Here's some possible additional documentation. Feedback appreciated.

diff --git a/lib/timeout.rb b/lib/timeout.rb
index a33bb4ce65..62a35169a4 100644
--- a/lib/timeout.rb
+++ b/lib/timeout.rb
@@ -67,7 +67,9 @@ def exception(*)
   # +sec+ seconds, otherwise throws an exception, based on the value of +klass+.
   #
   # The exception thrown to terminate the given block cannot be rescued inside
-  # the block unless +klass+ is given explicitly.
+  # the block unless +klass+ is given explicitly. However, the block can use
+  # ensure to prevent the handling of the exception.  For that reason, this
+  # method cannot be relied on to enforce timeouts for untrusted blocks.
   #
   # Note that this is both a method of module Timeout, so you can <tt>include
   # Timeout</tt> into your classes so they have a #timeout method, as well as

[moio (Silvio Moioli)]

I like this proposed wording, thanks for taking care of this!