Modify `yield timer` to `yield timeout` in `Scheduler#timeout_after`

[kudojp]

Description

Why

In the current implementation of Async::Scheduler#timeout_after, it executes the given block with the timer as the block argument as below.

    yield timer

(from. https://github.com/socketry/async/blob/v2.0.0/lib/async/scheduler.rb#L294)

However, in ruby 3.1, the scheduler which is set to the thread is triggered as below.

  def timeout(sec, klass = nil, message = nil, &block)   #:yield: +sec+
    # some lines are ommited here.

    if Fiber.respond_to?(:current_scheduler) && (scheduler = Fiber.current_scheduler)&.respond_to?(:timeout_after)
      return scheduler.timeout_after(sec, klass || Error, message, &block)
    end

(from. https://github.com/ruby/ruby/blob/ruby_3_1/lib/timeout.rb#L88-L90)

When both Timeout#timeout and Timeout.timeout is called, they take the block argument as timeout(second), not as a Timer object. This is also described in the official doc as below.

timeout(sec, klass = nil, message = nil) { |sec| ... }

from. https://docs.ruby-lang.org/en/3.1/Timeout.html

This should be fixed.

What

Async::Scheduler#timeout_after executes the given block with passing timeout as the argument.

    yield timeout

Types of Changes

• Bug fix.

Testing

• [ ] I added tests for my changes.
• [ ] I tested my changes locally.
• [ ] I tested my changes in staging.
• [ ] I tested my changes in production.

[kudojp]

Investigation of the impact of this change

I do grep search for places where timeout_after is used now.

Step 1. $ rg timeout_after

$ rg timeout_after

```rb lib/async/task.rb 95: # @deprecated Replaced by {Scheduler#timeout_after}. 97: Fiber.scheduler.timeout_after(timeout, exception, message, &block) lib/async/reactor.rb 37: alias with_timeout timeout_after lib/async/scheduler.rb 285: def timeout_after(timeout, exception = TimeoutError, message = "execution expired", &block) ```

I take a look at each of these.

1. lib/async/task.rb L95-L98

        # @deprecated Replaced by {Scheduler#timeout_after}.
        def with_timeout(timeout, exception = TimeoutError, message = "execution expired", &block)
            Fiber.scheduler.timeout_after(timeout, exception, message, &block)
        end

👉 When calling Task#with_timeout, the block is passed to the method call of Scheduler#with_timeout. This part crashes when Task#with_timeout is called with a block and the block argument is used. It should be a timeout, but now it is a timer. This has to be investigated more.

2. lib/async/reactor.rb L37

        alias with_timeout timeout_after

👉 Reactor class inherits Scheduler class and #timeout_after is the one defined as Scheduler#timeout_after. Thus, if Reactor#with_timeout or Reactor#timeout_after is called with a block and the block argument is used. It should be a timeout, but now it is a timer. This has to be investigated more.

3. lib/async/scheduler.rb L285

✅ This is exactly the method I update in this PR.

Both of 1 and 2 above are on #with_timeout method. I do grep search for with_timeout to see where they are used.

Step 2. rg with_timeout

$ rg with_timeout

```rb guides/getting-started/README.md 203: task.with_timeout(1) do lib/async/task.rb 96: def with_timeout(timeout, exception = TimeoutError, message = "execution expired", &block) spec/async/reactor_spec.rb 212: describe '#with_timeout' do 221: task.with_timeout(duration) do 244: task.with_timeout(0.0, timeout_class) do 253: x.report('Reactor#with_timeout') do |repeats| 258: reactor.with_timeout(1) do spec/async/task_spec.rb 365: describe '#with_timeout' do 368: task.with_timeout(0.2) do |timer| 387: task.with_timeout(0.01) do 407: task.with_timeout(0.01) do 426: task.with_timeout(1.0) do spec/async/semaphore_spec.rb 93: reactor.with_timeout(0.1) do spec/async/condition_examples.rb 85: task.with_timeout(0.1) do lib/async/reactor.rb 37: alias with_timeout timeout_after ```

In the result above, spec/async/task_spec.rb L368 is the only problem. I take a look.

spec/async/task_spec.rb L368

  describe '#with_timeout' do
    it "can extend timeout" do
      reactor.async do |task|
        task.with_timeout(0.2) do |timer|
          task.sleep(0.1)

          expect(timer.fires_in).to be_within(10 * Q).percent_of(0.1)

          timer.reset

          expect(timer.fires_in).to be_within(10 * Q).percent_of(0.2)
        end
      end

      reactor.run
    end

👉 I found from above that the timer of the block argument is used to reset/reschedule the timer associated with the task scheduled by Task#with_timeout. Since Task#with_timeout is now deprecated, it is acceptable to drop the capability to reset/reschedule these tasks.

[kudojp]

Mmm,,,

Now I noticed that if this PR is merged, resetting/rescheduling timers of the tasks which are scheduled by Scheduler#timeout_after becomes impossible too (even though it is not tested in any of the specs now).

[kudojp]

Hi @ioquatix, nice to meet you! I would like to know your idea!

In this PR, I updated Async::Scheduler#timeout_after so that Async::Scheduler becomes compatible with Timeout.timeout executed in Fiber.schedule block. However, with this change, the interface to reset/reschedule the tasks which are scheduled with Timeout.timeout would be lost.

details

In the current version, ```rb Fiber.set_scheduler Async::Scheduler.new Fiber.schedule do Timeout.timeout(10) do |t| # t is Timers::Timer object sleep(5) t.reset() # it is possible to reset the timer in this way end end ``` After the update of this PR, it becomes impossible. ```rb Fiber.set_scheduler Async::Scheduler.new Fiber.schedule do Timeout.timeout(10) do |t| # t is timeout(sec) sleep(20) end end ```

I would like to know if you think this is acceptable or not. (I have come up with solutions below and thought the solution2 is the best.)

Solution1. Give up to implement FiberScheduler interface completely

For this, we just have to close this PR. - Pros1. No job to be done. - Pros1. Executing `Timeout.timeout` in Fiber.schedule block with a block argument keeps crushing.

Solution2. Implement a FiberScheduler interface completely and accept to lose capability of reseting/rescheduling tasks

For this, we just have to merge this PR with one test case [#with_timeout - can extend timeout](https://github.com/socketry/async/blob/v2.0.0/spec/async/task_spec.rb#L365-L380) deleted. - Pros1. Executing `Timeout.timeout` in Fiber.schedule block with a block argument would not crush. - Cons1. In the next version of this gem, the interface to reset/reschedule tasks would not be provided.

Solution3. Implement FiberScheduler interface completely and add another interface to reset/reschedule tasks.

For this, we have to update this PR more. - Pros1. Executing `Timeout.timeout` in Fiber.schedule block with a block argument would not crush. - Pros2. The interface to reset/reschedule tasks would keep provided. - Cons1. This costs much since we have to think from the design. - Cons2. If the interface to reset/reschedule tasks is defined in FiberScheduler in the future, we have to implement again.

I personally think solution 2 is the best. If the capability of resetting/rescheduling tasks is necessary, I think updating the interface of FiberScheduler should come first. If the Ruby maintainers (including you) make a conclusion that reseting/rescheduling tasks is useful, the interface should also be decided there. After that, this gem should be updated in that way, I think.

[ioquatix]

Thanks for all the details and notes. I will implement option 3 or some variant of it as the interface should be compatible between Async 1 and 2. I have a locally working branch I'll push it later this week.

[ioquatix]

Thanks I have merged a fix for this.

[kudojp]

Thanx!

[ioquatix]

I will try to release this soon after I test the changes.