From 3683d2d251889865334f861791e5e743dca48898 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 20 Aug 2019 21:09:04 +0200
Subject: [PATCH] Perform cheap thread find

If we process message that is not designated to us
previously we would fire a separate Thread for that.
We don't need to do it. We can cheaply check if thread
is available, if it is, we can perform expensive operation
then.
---
 lib/gitlab/sidekiq_monitor.rb           | 25 ++++++++++++++++---------
 spec/lib/gitlab/sidekiq_monitor_spec.rb |  4 +---
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/sidekiq_monitor.rb b/lib/gitlab/sidekiq_monitor.rb
index 0d4709508a0..bfbf998b3a0 100644
--- a/lib/gitlab/sidekiq_monitor.rb
+++ b/lib/gitlab/sidekiq_monitor.rb
@@ -110,11 +110,13 @@ module Gitlab
     def process_job_cancel(jid)
       return unless jid
 
-      # since this might take time, process cancel in a new thread
-      Thread.new do
-        find_thread(jid) do |thread|
-          next unless thread
+      # try to find thread without lock
+      return unless find_thread_unsafe(jid)
 
+      Thread.new do
+        # try to find a thread, but with guaranteed
+        # handle that this thread corresponds to actually running job
+        find_thread_with_lock(jid) do |thread|
           Sidekiq.logger.warn(
             class: self.class,
             action: 'cancel',
@@ -130,13 +132,18 @@ module Gitlab
     # This method needs to be thread-safe
     # This is why it passes thread in block,
     # to ensure that we do process this thread
-    def find_thread(jid)
-      return unless jid
+    def find_thread_unsafe(jid)
+      jobs_thread[jid]
+    end
+
+    def find_thread_with_lock(jid)
+      # don't try to lock if we cannot find the thread
+      return unless find_thread_unsafe(jid)
 
       jobs_mutex.synchronize do
-        thread = jobs_thread[jid]
-        yield(thread)
-        thread
+        find_thread_unsafe(jid).tap do |thread|
+          yield(thread) if thread
+        end
       end
     end
 
diff --git a/spec/lib/gitlab/sidekiq_monitor_spec.rb b/spec/lib/gitlab/sidekiq_monitor_spec.rb
index 7c9fc598b02..f1804bd0755 100644
--- a/spec/lib/gitlab/sidekiq_monitor_spec.rb
+++ b/spec/lib/gitlab/sidekiq_monitor_spec.rb
@@ -145,9 +145,7 @@ describe Gitlab::SidekiqMonitor do
       context 'when jid is not found' do
         it 'does not log cancellation message' do
           expect(Sidekiq.logger).not_to receive(:warn)
-          expect(subject).to be_a(Thread)
-
-          subject.join
+          expect(subject).to be_nil
         end
       end