Implement review comments from @rymai and @yorickpeterse

1. Refactoring and specs in the `Uniquify` class.

2. Don't use the `AdvisoryLocking` class. Similar functionality is
provided (backed by Redis) in the `ExclusiveLease` class.

4 files changed, 61 insertions, 81 deletions

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
index 1485ab6ae99..6734472bc6d 100644
--- a/app/models/concerns/uniquify.rb
+++ b/app/models/concerns/uniquify.rb
@@ -6,19 +6,23 @@ class Uniquify
   # If `base` is a function/proc, we expect that calling it with a
   # candidate counter returns a string to test/return.
   def string(base, exists_fn)
+    @base = base
     @counter = nil
 
-    if base.respond_to?(:call)
-      increment_counter! while exists_fn[base.call(@counter)]
-      base.call(@counter)
-    else
-      increment_counter! while exists_fn["#{base}#{@counter}"]
-      "#{base}#{@counter}"
-    end
+    increment_counter! while exists_fn[base_string]
+    base_string
   end
 
   private
 
+  def base_string
+    if @base.respond_to?(:call)
+      @base.call(@counter)
+    else
+      "#{@base}#{@counter}"
+    end
+  end
+
   def increment_counter!
     @counter = @counter ? @counter.next : 1
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index f71d7e90d33..7207cdc4581 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,9 +351,15 @@ class User < ActiveRecord::Base
       ghost_user ||
         begin
           # Since we only want a single ghost user in an instance, we use an
-          # advisory lock to ensure than this block is never run concurrently.
-          advisory_lock = Gitlab::Database::AdvisoryLocking.new(:ghost_user)
-          advisory_lock.lock
+          # exclusive lease to ensure than this block is never run concurrently.
+          lease_key = "ghost_user_creation"
+          lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
+
+          until uuid = lease.try_obtain
+            # Keep trying until we obtain the lease. To prevent hammering Redis too
+            # much we'll wait for a bit between retries.
+            sleep(1)
+          end
 
           # Recheck if a ghost user is already present (one might have been)
           # added between the time we last checked (first line of this method)
@@ -375,7 +381,7 @@ class User < ActiveRecord::Base
             email: email, name: "Ghost User", state: :blocked, ghost: true
           )
         ensure
-          advisory_lock.unlock
+          Gitlab::ExclusiveLease.cancel(lease_key, uuid)
         end
     end
   end
diff --git a/lib/gitlab/database/advisory_locking.rb b/lib/gitlab/database/advisory_locking.rb
deleted file mode 100644
index 950898288be..00000000000
--- a/lib/gitlab/database/advisory_locking.rb
+++ /dev/null
@@ -1,70 +0,0 @@
-# An advisory lock is an application-level database lock which isn't tied
-# to a specific table or row.
-#
-# Postgres names its advisory locks with integers, while MySQL uses strings.
-# We support both here by using a `LOCK_TYPES` map of symbols to integers.
-# The symbol (stringified) is used for MySQL, and the corresponding integer
-# is used for Postgres.
-module Gitlab
-  module Database
-    class AdvisoryLocking
-      LOCK_TYPES = {
-        ghost_user: 1
-      }
-
-      def initialize(lock_type)
-        @lock_type = lock_type
-      end
-
-      def lock
-        ensure_valid_lock_type!
-
-        query =
-          if Gitlab::Database.postgresql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("pg_advisory_lock", [LOCK_TYPES[@lock_type]])
-            )
-          elsif Gitlab::Database.mysql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("get_lock", [Arel.sql("'#{@lock_type}'"), -1])
-            )
-          end
-
-        run_query(query)
-      end
-
-      def unlock
-        ensure_valid_lock_type!
-
-        query =
-          if Gitlab::Database.postgresql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("pg_advisory_unlock", [LOCK_TYPES[@lock_type]])
-            )
-          elsif Gitlab::Database.mysql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("release_lock", [Arel.sql("'#{@lock_type}'")])
-            )
-          end
-
-        run_query(query)
-      end
-
-      private
-
-      def ensure_valid_lock_type!
-        unless valid_lock_type?
-          raise RuntimeError, "Trying to use an advisory lock with an invalid lock type, #{@lock_type}."
-        end
-      end
-
-      def valid_lock_type?
-        LOCK_TYPES.keys.include?(@lock_type)
-      end
-
-      def run_query(arel_query)
-        ActiveRecord::Base.connection.execute(arel_query.to_sql)
-      end
-    end
-  end
-end
diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
new file mode 100644
index 00000000000..4e06a1f4c31
--- /dev/null
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe Uniquify, models: true do
+  describe "#string" do
+    it 'returns the given string if it does not exist' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { false })
+
+      expect(result).to eq('test_string')
+    end
+
+    it 'returns the given string with a counter attached if the string exists' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
+
+      expect(result).to eq('test_string1')
+    end
+
+    it 'increments the counter for each candidate string that also exists' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
+
+      expect(result).to eq('test_string2')
+    end
+
+    it 'allows passing in a base function that defines the location of the counter' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string(
+        -> (counter) { "test_#{counter}_string" },
+        -> (s) { true if s == 'test__string' }
+      )
+
+      expect(result).to eq('test_1_string')
+    end
+  end
+end