Properly shut down Concurrent::TimerTasks when Puma is stopped (#4681)

* Properly shut down Concurrent::TimerTasks when Puma is stopped

* Spec for PeriodicUpdater.stop_updates

Author: jochenehret

lib/cloud_controller/metrics/periodic_updater.rb

@@ -16,15 +16,23 @@ def initialize(start_time, log_counter, logger, statsd_updater, prometheus_updat
 
     def setup_updates
       update!
-      Concurrent::TimerTask.new(execution_interval: 600) { catch_error { update_user_count } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_job_queue_length } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_job_queue_load } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_failed_job_count } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_vitals } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_log_counts } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_task_stats } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_deploying_count } }.execute
-      Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_webserver_stats } }.execute
+      @update_tasks = []
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 600) { catch_error { update_user_count } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_job_queue_length } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_job_queue_load } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_failed_job_count } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_vitals } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_log_counts } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_task_stats } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_deploying_count } }
+      @update_tasks << Concurrent::TimerTask.new(execution_interval: 30) { catch_error { update_webserver_stats } }
+      @update_tasks.each(&:execute)
+    end
+
+    def stop_updates
+      return unless @update_tasks
+
+      @update_tasks.each(&:shutdown)
     end
 
     def update!

lib/cloud_controller/runners/puma_runner.rb

@@ -6,6 +6,7 @@
 module VCAP::CloudController
   class PumaRunner
     def initialize(config, app, logger, periodic_updater, request_logs)
+      @periodic_updater = periodic_updater
       @logger = logger
 
       ENV['WEB_CONCURRENCY'] = 'auto' if config.get(:puma, :automatic_worker_count)
@@ -59,8 +60,12 @@ def initialize(config, app, logger, periodic_updater, request_logs)
       events = Puma::Events.new
       events.after_booted do
         prometheus_updater.update_gauge_metric(:cc_db_connection_pool_timeouts_total, 0, labels: { process_type: 'main' })
-        periodic_updater.setup_updates
+        @periodic_updater.setup_updates
       end
+      events.after_stopped do
+        @periodic_updater.stop_updates unless @periodic_updater.nil?
+      end
+
       @puma_launcher = Puma::Launcher.new(puma_config, log_writer:, events:)
     end
 

spec/unit/lib/cloud_controller/metrics/periodic_updater_spec.rb

@@ -183,6 +183,28 @@ module VCAP::CloudController::Metrics
           @periodic_timers[6][:block].call
         end
       end
+
+      context 'when PeriodicUpdater is shut down' do
+        it 'does nothing when updates have not been setup' do
+          expect { periodic_updater.stop_updates }.not_to raise_error
+        end
+
+        it 'shuts down all timer tasks after setup_updates' do
+          timer_doubles = []
+          allow(Concurrent::TimerTask).to receive(:new) do |*|
+            dbl = double('TimerTask', execute: nil, shutdown: nil)
+            timer_doubles << dbl
+            dbl
+          end
+
+          periodic_updater.setup_updates
+
+          expect(timer_doubles.size).to eq(9)
+          expect(timer_doubles).to all(receive(:shutdown).once)
+
+          periodic_updater.stop_updates
+        end
+      end
     end
 
     describe '#update_deploying_count' do

spec/unit/lib/cloud_controller/runners/puma_runner_spec.rb

@@ -204,7 +204,9 @@ module VCAP::CloudController
       end
 
       describe 'after_stopped' do
-        it 'stops EM and logs incomplete requests' do
+        it 'stops the TimerTasks and logs incomplete requests' do
+          expect(periodic_updater).to receive(:stop_updates)
+
           puma_launcher.events.fire(:after_stopped)
         end
       end