Thread.handle_interrupt results in an unpredictable behavior since #15

[akihikodaki]

tl; dr

Thread.handle_interrupt won't work well when the timeout thread is reused. It is because blocked interrupts are inherited when the thread is created. In my opinion such an inheritance should not happen, but removing it will be a breaking change and requires core Ruby change.

Long story

#15 changed Timeout to reuse a thread. On the other hand, a thread inherits the blocked interrupts of the creating thread. This means the list of blocked interrupts will persist once after a thread is created first time.

This breaks the code something like follows:

# Part X
timeout(10) {}

# Part Y
Thread.handle_interrupt(Timeout::Error => :never) do
  timeout(10) do
    Thread.handle_interrupt(Timeout::Error => :on_blocking) do
      do_something
    end
  ensure
    clean # Timeout::Error shouldn't occur here (but it can since #15)
  end
end

Code Y follows the example shown at: https://docs.ruby-lang.org/en/3.2/Thread.html#method-c-handle_interrupt-label-Guarding+from+Timeout-3A-3AError
In this case, the later Thread.handle_interrupt(Timeout::Error => :never) will have no effect.

The opposite scenario can also happen. Consider the following code:

# Part Y
Thread.handle_interrupt(Timeout::Error => :never) do
  timeout(10) do
    Thread.handle_interrupt(Timeout::Error => :on_blocking) do
      do_something
    end
  ensure
    clean
  end
end

# Part X
timeout(10) do
  # Timeout::Error should happen here but it won't in reality.
end

In my opinion, this kind of problem occurs due to a bad design of Ruby threads; a new thread should not inherit the blocked interrupts. The inheritance of blocked interrupts can break multithreading code so easily. Thread reuse as seen in Timeout is one of patterns affected by this design. Any kind of thread pools will also be affected.

The inheritance is also problematic when a method called from Thread.handle_interrupt block uses Thread#raise. For example, think of the following code:

module SomeExternalLibrary
  def gracefully_close_connection
    begin
      timeout(10) { tell_the_remote_machine_that_i_am_leaving }
    rescue Timeout::Error
      # I guess the remote machine is dead.
    end
    @@socket.close
  end
end

# I want to make sure graceful shutdown happens even when SIGINT happens.
Thread.handle_interrupt(Object => :never) do
  SomeExternalLibrary.gracefully_close_connection
end

SomeExternalLibrary.gracefully_close_connection may not work because timeout is blocked with Thread.handle_interrupt(Object => :never).

This kind of problems will not happen if the inheritance of blocked interrupts does not happen. Removing the inheritance of blocked interrupts would break code part Y, but I don't think that matters much. In fact, if you should be able to rewrite the code as follows:

begin
  timeout(10) do
    do_something
  end
ensure
  clean # Here Timeout::Error won't happen because it's out of the timeout block.
end

If you still need to block Timeout::Error at the beginning of the timeout block, timeout method can have an extra parameter to block interrupts before the timer gets armed though I don't see a use case for that.

That said, removing the inheritance of blocked interrupts will be a breaking change so it's not really practical. Also it will be a core Ruby change so timeout gem may need its own solution, perhaps just reverting #15, adding an option, or just edit the documentation to say "do not use timeout with Thread.handle_interrupt ever".