Cable: Periodic timers refresh

* Rewrite docs
* Support blocks in addition to method names and Proc args
* Check for valid arguments
* Convert `periodically :method_name` to Proc callbacks
* Drop periodic runner methods from the worker pool
* Ensure we clear active periodic timers after shutdown

2 files changed, 38 insertions, 20 deletions

diff --git a/actioncable/test/channel/periodic_timers_test.rb b/actioncable/test/channel/periodic_timers_test.rb
index e6f0c14c9d..03464003cf 100644
--- a/actioncable/test/channel/periodic_timers_test.rb
+++ b/actioncable/test/channel/periodic_timers_test.rb
@@ -1,12 +1,21 @@
 require 'test_helper'
 require 'stubs/test_connection'
 require 'stubs/room'
+require 'active_support/time'
 
 class ActionCable::Channel::PeriodicTimersTest < ActiveSupport::TestCase
   class ChatChannel < ActionCable::Channel::Base
-    periodically -> { ping }, every: 5
+    # Method name arg
     periodically :send_updates, every: 1
 
+    # Proc arg
+    periodically -> { ping }, every: 2
+
+    # Block arg
+    periodically every: 3 do
+      ping
+    end
+
     private
       def ping
       end
@@ -19,22 +28,41 @@ class ActionCable::Channel::PeriodicTimersTest < ActiveSupport::TestCase
   test "periodic timers definition" do
     timers = ChatChannel.periodic_timers
 
-    assert_equal 2, timers.size
+    assert_equal 3, timers.size
 
-    first_timer = timers[0]
-    assert_kind_of Proc, first_timer[0]
-    assert_equal 5, first_timer[1][:every]
+    timers.each_with_index do |timer, i|
+      assert_kind_of Proc, timer[0]
+      assert_equal i+1, timer[1][:every]
+    end
+  end
 
-    second_timer = timers[1]
-    assert_equal :send_updates, second_timer[0]
-    assert_equal 1, second_timer[1][:every]
+  test 'disallow negative and zero periods' do
+    [ 0, 0.0, 0.seconds, -1, -1.seconds, 'foo', :foo, Object.new ].each do |invalid|
+      assert_raise ArgumentError, /Expected every:/ do
+        ChatChannel.periodically :send_updates, every: invalid
+      end
+    end
+  end
+
+  test 'disallow block and arg together' do
+    assert_raise ArgumentError, /not both/ do
+      ChatChannel.periodically(:send_updates, every: 1) { ping }
+    end
+  end
+
+  test 'disallow unknown args' do
+    [ 'send_updates', Object.new, nil ].each do |invalid|
+      assert_raise ArgumentError, /Expected a Symbol/ do
+        ChatChannel.periodically invalid, every: 1
+      end
+    end
   end
 
   test "timer start and stop" do
-    @connection.server.event_loop.expects(:timer).times(2).returns(true)
+    @connection.server.event_loop.expects(:timer).times(3).returns(stub(shutdown: nil))
     channel = ChatChannel.new @connection, "{id: 1}", { id: 1 }
 
-    channel.expects(:stop_periodic_timers).once
     channel.unsubscribe_from_channel
+    assert_equal [], channel.send(:active_periodic_timers)
   end
 end
diff --git a/actioncable/test/worker_test.rb b/actioncable/test/worker_test.rb
index 654f49821e..7016da3493 100644
--- a/actioncable/test/worker_test.rb
+++ b/actioncable/test/worker_test.rb
@@ -41,14 +41,4 @@ class WorkerTest < ActiveSupport::TestCase
     @worker.invoke @receiver, :process, "Hello"
     assert_equal [ :process, "Hello" ], @receiver.last_action
   end
-
-  test "running periodic timers with a proc" do
-    @worker.run_periodic_timer @receiver, @receiver.method(:run)
-    assert_equal :run, @receiver.last_action
-  end
-
-  test "running periodic timers with a method" do
-    @worker.run_periodic_timer @receiver, :run
-    assert_equal :run, @receiver.last_action
-  end
 end