fix: Stop repeating tasks promptly instead of waiting out the sleep (#432)

Author: jsonbailey

lib/ldclient-rb/impl/repeating_task.rb

@@ -4,22 +4,39 @@
 
 module LaunchDarkly
   module Impl
+    #
+    # Runs a task again and again on a worker thread, with a fixed interval
+    # between runs.
+    #
+    # The worker waits on an event instead of calling `sleep`, so `stop` can
+    # wake it at once even if `stop` runs before the worker starts waiting.
+    #
+    # @private
+    #
     class RepeatingTask
       attr_reader :name
 
+      #
+      # @param interval [Numeric] seconds between the start of one run and the start of the next
+      # @param start_delay [Numeric, nil] seconds to wait before the first run
+      # @param task [Proc] the code to run
+      # @param logger [Logger]
+      # @param name [String] the name given to the worker thread
+      #
       def initialize(interval, start_delay, task, logger, name)
         @interval = interval
         @start_delay = start_delay
         @task = task
         @logger = logger
         @stopped = Concurrent::AtomicBoolean.new(false)
+        @stop_event = Concurrent::Event.new
         @worker = nil
         @name = name
       end
 
       def start
         @worker = Thread.new do
-          sleep(@start_delay) unless @start_delay.nil? || @start_delay == 0
+          @stop_event.wait(@start_delay) unless @start_delay.nil? || @start_delay == 0
 
           until @stopped.value do
             started_at = Time.now
@@ -29,19 +46,23 @@ def start
               Impl::Util.log_exception(@logger, "Uncaught exception from repeating task", e)
             end
             delta = @interval - (Time.now - started_at)
-            if delta > 0
-              sleep(delta)
-            end
+            @stop_event.wait(delta) if delta > 0
           end
         end
 
         @worker.name = @name
       end
 
+      #
+      # Stops the worker thread and waits for it to finish.
+      #
+      # This method is safe to call more than once, before `start`, and from
+      # inside the task itself.
+      #
       def stop
         if @stopped.make_true
+          @stop_event.set
           if @worker && @worker.alive? && @worker != Thread.current
-            @worker.run  # causes the thread to wake up if it's currently in a sleep
             @worker.join
           end
         end

spec/impl/repeating_task_spec.rb

@@ -59,6 +59,79 @@ def null_logger
         expect(no_more_items).to be true
       end
 
+      it "stops promptly when stopped during a long start delay" do
+        ran = Concurrent::Event.new
+        task = RepeatingTask.new(10, 10, -> { ran.set }, null_logger, "test")
+        task.start
+        started_at = Time.now
+        task.stop
+        elapsed = Time.now - started_at
+
+        expect(elapsed).to be < 1
+        expect(ran.set?).to be false
+      end
+
+      it "stops promptly when stopped while waiting between runs" do
+        ran = Concurrent::Event.new
+        task = RepeatingTask.new(10, 0, -> { ran.set }, null_logger, "test")
+        begin
+          task.start
+          expect(ran.wait(1)).to be true
+          started_at = Time.now
+          task.stop
+          elapsed = Time.now - started_at
+
+          expect(elapsed).to be < 1
+        ensure
+          task.stop
+        end
+      end
+
+      it "can be stopped before it is started" do
+        task = RepeatingTask.new(0.01, 0, -> {}, null_logger, "test")
+
+        expect { task.stop }.not_to raise_error
+      end
+
+      it "does not run the task if stopped before it is started" do
+        ran = Concurrent::Event.new
+        task = RepeatingTask.new(0.01, 0, -> { ran.set }, null_logger, "test")
+        task.stop
+        task.start
+        begin
+          expect(ran.wait(0.1)).to be false
+        ensure
+          task.stop
+        end
+      end
+
+      it "can be stopped more than once" do
+        task = RepeatingTask.new(10, 0, -> {}, null_logger, "test")
+        task.start
+        task.stop
+
+        expect { task.stop }.not_to raise_error
+      end
+
+      it "keeps running after the task raises an exception" do
+        queue = Queue.new
+        calls = 0
+        task = RepeatingTask.new(0.01, 0,
+          -> {
+            calls += 1
+            queue << calls
+            raise "boom" if calls == 1
+          },
+          null_logger, "test")
+        begin
+          task.start
+          expect(queue.pop).to eq(1)
+          expect(queue.pop).to eq(2)
+        ensure
+          task.stop
+        end
+      end
+
       it "can be stopped from within the task" do
         counter = 0
         stopped = Concurrent::Event.new