Timeout extensions

[HoneyryderChuck]

Continuation of what's going on here. This is how I envisioned integrating the extensions into celluloid. Main changes are a few Exceptions, method signatures and the fact that the main timeout and sleep logic is directly inside the Celluloid module, instead of inside the actor.

Main discussion topics:

1. I think that the Actor should drop the timeout and sleep instance methods. I mean, they should be using timeout and sleep directly, which will revert to the handlers inside its thread. This will not be a breaking change for the ones who have been using timeout and sleep inside the actor scope as if they were a private method, but it would break such usages as Thread.current[:celluloid_actor].timeout(duration)...:
2. I think that one should drop Celluloid::Task::TimeoutError in favour of Timeout::Error. I mean, one should use the same exceptions as other potential libraries expect (Net::SSH, Net::HTTP, etc, they all work with it). I think one did it because of the ResumableError class, but since this has become a module, one could "extend" the Timeout::Error instance before resuming the task (see how this was solved under Celluloid.timeout).
3. A somewhat elephant in the room is Celluloid.sleep argument handling. I couldn't really mimic the Kernel.sleep method signature, as you can call sleep without arguments. The question here is, should one allow celluloid actors to sleep forever? Does the timers gem support this feature? Do you think this is a relevant use-case? I think one should at least throw a relevant exception for the case in which no argument is passed to a sleep evaluated inside an actor.

[tarcieri]

Need to get timeout-extensions working on Celluloid master first

[HoneyryderChuck]

@tarcieri , I've noticed the celluloid refactorings recently. Are the timeout extensions being considered for any of the planned minor releases?

[tarcieri]

@TiagoCardoso1983 given how invasive they are, I think I'd prefer to keep them out of core if possible. Perhaps this patch could be further modularized so that less of it depends on changes to Celluloid.

[HoneyryderChuck]

I would argue that these changes are not that invasive, but correct some fundamental flaws to the "time" methods. For instance, Celluloid.timeout currently takes one argument instead of two, like ruby's Timeout::timeout. Besides that I don't see what you mean by invasive.

[HoneyryderChuck]

Here is the all necessary monkey-patching needed for the extensions to work:

module Celluloid
  class Thread < ::Thread
    def initialize(*)
      self.timeout_handler = Celluloid.method(:timeout).to_proc
      self.sleep_handler = Celluloid::method(:sleep).to_proc
      super
    end
  end

  def self.timeout(duration, klass = nil)
    bt = caller
    task = Task.current
    klass ||= Task::TimeoutError
    timers = Thread.current[:celluloid_actor].timers
    timer = timers.after(duration) do
      exception = klass.new("execution expired")
      exception.set_backtrace bt
      task.resume exception
    end
    yield
  ensure
    timer.cancel if timer
  end

  class Actor
    def timeout(*args)
      Celluloid::timeout(*args) { yield }
    end
    private :timeout
  end


  remove_const(:ResumableError)

  module ResumableError ; end

  class Task
    remove_const(:TerminatedError)
    remove_const(:TimeoutError)
    class TerminatedError < Celluloid::Error; include ResumableError ;end
    class TimeoutError < Celluloid::Error; include ResumableError ;end
  end

end

I could indeed puts this into a timeout-extensions celluloid script, but this looks like API which can be subject to change, and actually everything that timeout-extensions needs to work are those patches in the Thread class. The rest of it are just problems I had while using Celluloid::IO with some classes which call timeout with 2 arguments.

[HoneyryderChuck]

@digitalextremist could you review this? This was under discussion with @tarcieri , but was forgotten in the whole 0.17 migration and was a bit in the limbo. Nevertheless, this addresses the timeout issue discussed under the sidekiq thread (Namely that plain ruby timeout calls break actors).

There is currently one failing spec, and this has to due with the (again, discussed somewhere else) fact that TaskTimeout exceptions now inherit from Exception and can't be caught by the RSpec raise_error call (and again, this could also be the initial point for a rediscussion on that design decision, as I'm not particularly a fan of it).

[digitalextremist]

@TiagoCardoso1983 no problem. Will review.

[ioquatix]

What are the performance implications of this additional layer of indirection? RubyDNS uses timeouts a lot in performance sensitive code for handling the (rare) case where the network doesn't respond in time. The overhead for timeouts that don't fire and are cancelled needs to be minimised at the expense of timeouts that do eventually fire, IMHO.

[HoneyryderChuck]

@ioquatix I'm not sure that I understood. This pull request doesn't change the way celluloid works, it just "moves" the timeout BL to the Celluloid module. Why? Because it makes it easier to use here.

I would argue that network not respond in time is not rare, and in the context of celluloid, you'd need to prevent the usage of the ruby timeout/sleep, which will block the actor message-processing. In the context of celluloid-io, which can be used to patch stdlib and other network libraries, which use Timeout.timeout extensively, that's even more important.

[ioquatix]

Okay I will review.

[HoneyryderChuck]

As you may have seen by now, this was applied to an ancient version of celluloid. It has been scattered across subgems as of 0.17, which makes this harder to track. What concerns celluloid, the parts I'd like to maintain in the pull request are migrating the BL of timeout to Celluloid module, where Celluloid.sleep is. I guess, since they're timer based tasks, it makes sense to have them close to each other and publicly acessible.

The point of overhead you might have noticed comes from the thread pool spawner. timeout-extensions defines two internal accessor methods to keep pointers to an alternative thread-local timeout/sleep implementation (it doesn't need to be celluloid-specific). Therefore, on thread spawing, Celluloid timeout/sleep are passed to the thread. timeout-extensions proceeds to "monkey-patch" Timeout.timeout and Kernel.sleep (when the thread has the method locally-assigned, it calls it).

This timeout/sleep overwrites have been discussed ages ago with @tarcieri . He was never fond of "monkey-patching" Kernel and Timeout.timeout (who is?), rather wanted to "push" the community to adopt the approach. I decided not to wait and have been using it in production in my company internal projects without significant issues (main issues: 1) Celluloid::Task::TimeoutError inherits from Exception instead of StandardError/Timeout::Error, makes rescu'ing a bitx; and 2) every call to sleep/timeout will have timeout-extensions in the stack trace, prompting users to single it out whenever they have issues with timers, although the ruby timeout/sleep are ultimately executed).

[ioquatix]

I completely agree with you on all points. We will try to resolve these issues for 0.18.0 including merging your changes.

[HoneyryderChuck]

No worries, I just wanted to give you the history of this issue.

[tarcieri]

@HoneyryderChuck not sure if you're interested in picking this up. Many of the subgems are now unified again, which should make this change easier.

[HoneyryderChuck]

@tarcieri haven't been using celluloid for a while, so this stopped becoming a priority for me. Besides that, I think the PR is sort of complete from my side, so it's a matter of it being worthy of being merged or not. I'm actually surprised there are no conflicts, given how much has changed...

[tarcieri]

Absent a volunteer to carry this forward, I don't see this getting merged.

If someone would like to continue this work, please create a new, up-to-date PR.