Merge branch 'make-daemon-really-thread-safe' into 'master'

Make Daemon thread-safe

See merge request gitlab-org/gitlab!18575

lib/gitlab/daemon.rb

@@ -34,7 +34,9 @@ module Gitlab
       @mutex.synchronize do
         break thread if thread?
 
-        @thread = Thread.new { start_working }
+        if start_working
+          @thread = Thread.new { run_thread }
+        end
       end
     end
 
@@ -57,10 +59,18 @@ module Gitlab
 
     private
 
+    # Executed in lock context before starting thread
+    # Needs to return success
     def start_working
+      true
+    end
+
+    # Executed in separate thread
+    def run_thread
       raise NotImplementedError
     end
 
+    # Executed in lock context
     def stop_working
       # no-ops
     end

lib/gitlab/metrics/exporter/base_exporter.rb

@@ -40,7 +40,14 @@ module Gitlab
               ::Gitlab::HealthChecks::Probes::Liveness.new, req, res)
           end
           server.mount '/', Rack::Handler::WEBrick, rack_app
-          server.start
+
+          true
+        end
+
+        def run_thread
+          server&.start
+        rescue IOError
+          # ignore forcibily closed servers
         end
 
         def stop_working

lib/gitlab/metrics/samplers/base_sampler.rb

@@ -50,6 +50,11 @@ module Gitlab
 
         def start_working
           @running = true
+
+          true
+        end
+
+        def run_thread
           sleep(sleep_interval)
           while running
             safe_sample

lib/gitlab/sidekiq_daemon/memory_killer.rb

@@ -29,7 +29,7 @@ module Gitlab
 
       private
 
-      def start_working
+      def run_thread
         Sidekiq.logger.info(
           class: self.class.to_s,
           action: 'start',

lib/gitlab/sidekiq_daemon/monitor.rb

@@ -61,7 +61,7 @@ module Gitlab
 
       private
 
-      def start_working
+      def run_thread
         return unless notification_channel_enabled?
 
         begin

spec/lib/gitlab/daemon_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Daemon do
   subject { described_class.new }
 
   before do
-    allow(subject).to receive(:start_working)
+    allow(subject).to receive(:run_thread)
     allow(subject).to receive(:stop_working)
   end
 
@@ -44,7 +44,7 @@ describe Gitlab::Daemon do
         it 'starts the Daemon' do
           expect { subject.start.join }.to change { subject.thread? }.from(false).to(true)
 
-          expect(subject).to have_received(:start_working)
+          expect(subject).to have_received(:run_thread)
         end
       end
 
@@ -52,7 +52,21 @@ describe Gitlab::Daemon do
         it "doesn't shutdown stopped Daemon" do
           expect { subject.stop }.not_to change { subject.thread? }
 
-          expect(subject).not_to have_received(:start_working)
+          expect(subject).not_to have_received(:run_thread)
+        end
+      end
+    end
+
+    describe '#start_working' do
+      context 'when start_working fails' do
+        before do
+          expect(subject).to receive(:start_working) { false }
+        end
+
+        it 'does not start thread' do
+          expect(subject).not_to receive(:run_thread)
+
+          expect(subject.start).to eq(nil)
         end
       end
     end
@@ -66,7 +80,7 @@ describe Gitlab::Daemon do
         it "doesn't start running Daemon" do
           expect { subject.start.join }.not_to change { subject.thread }
 
-          expect(subject).to have_received(:start_working).once
+          expect(subject).to have_received(:run_thread).once
         end
       end
 
@@ -79,7 +93,7 @@ describe Gitlab::Daemon do
 
         context 'when stop_working raises exception' do
           before do
-            allow(subject).to receive(:start_working) do
+            allow(subject).to receive(:run_thread) do
               sleep(1000)
             end
           end
@@ -108,7 +122,7 @@ describe Gitlab::Daemon do
         expect(subject.start).to be_nil
         expect { subject.start }.not_to change { subject.thread? }
 
-        expect(subject).not_to have_received(:start_working)
+        expect(subject).not_to have_received(:run_thread)
       end
     end
 

spec/lib/gitlab/sidekiq_daemon/memory_killer_spec.rb

@@ -12,8 +12,8 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do
     allow(Sidekiq.logger).to receive(:warn)
   end
 
-  describe '#start_working' do
-    subject { memory_killer.send(:start_working) }
+  describe '#run_thread' do
+    subject { memory_killer.send(:run_thread) }
 
     before do
       # let enabled? return 3 times: true, true, false
@@ -37,7 +37,7 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do
           .with(
             class: described_class.to_s,
             pid: pid,
-            message: "Exception from start_working: My Exception")
+            message: "Exception from run_thread: My Exception")
 
         expect(memory_killer).to receive(:rss_within_range?).twice.and_raise(StandardError, 'My Exception')
         expect(memory_killer).to receive(:sleep).twice.with(Gitlab::SidekiqDaemon::MemoryKiller::CHECK_INTERVAL_SECONDS)
@@ -50,7 +50,7 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do
           .with(
             class: described_class.to_s,
             pid: pid,
-            message: "Exception from start_working: My Exception")
+            message: "Exception from run_thread: My Exception")
 
         expect(memory_killer).to receive(:rss_within_range?).once.and_raise(Exception, 'My Exception')
 

spec/lib/gitlab/sidekiq_daemon/monitor_spec.rb

@@ -37,8 +37,8 @@ describe Gitlab::SidekiqDaemon::Monitor do
     end
   end
 
-  describe '#start_working when notification channel not enabled' do
-    subject { monitor.send(:start_working) }
+  describe '#run_thread when notification channel not enabled' do
+    subject { monitor.send(:run_thread) }
 
     it 'return directly' do
       allow(monitor).to receive(:notification_channel_enabled?).and_return(nil)
@@ -52,8 +52,8 @@ describe Gitlab::SidekiqDaemon::Monitor do
     end
   end
 
-  describe '#start_working when notification channel enabled' do
-    subject { monitor.send(:start_working) }
+  describe '#run_thread when notification channel enabled' do
+    subject { monitor.send(:run_thread) }
 
     before do
       # we want to run at most once cycle