From f2ed82fa8486875660b80dd061827ac8b86d00b6 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 16 Feb 2017 12:35:10 +0530 Subject: [PATCH] Implement final review comments from @DouweM and @rymai - Have `Uniquify` take a block instead of a Proc/function. This is more idiomatic than passing around a function in Ruby. - Block a user before moving their issues to the ghost user. This avoids a data race where an issue is created after the issues are migrated to the ghost user, and before the destroy takes place. - No need to migrate issues (to the ghost user) in a transaction, because we're using `update_all` - Other minor changes --- app/models/concerns/uniquify.rb | 9 ++-- app/models/namespace.rb | 2 +- app/models/user.rb | 72 +++++++++++++-------------- app/services/users/destroy_service.rb | 13 +++-- spec/models/concerns/uniquify_spec.rb | 23 +++------ spec/services/users/destroy_spec.rb | 4 ++ 6 files changed, 62 insertions(+), 61 deletions(-) diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb index 6734472bc6d..a7fe5951b6e 100644 --- a/app/models/concerns/uniquify.rb +++ b/app/models/concerns/uniquify.rb @@ -1,15 +1,15 @@ class Uniquify # Return a version of the given 'base' string that is unique # by appending a counter to it. Uniqueness is determined by - # repeated calls to `exists_fn`. + # repeated calls to the passed block. # # 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) + def string(base) @base = base @counter = nil - increment_counter! while exists_fn[base_string] + increment_counter! while yield(base_string) base_string end @@ -24,6 +24,7 @@ class Uniquify end def increment_counter! - @counter = @counter ? @counter.next : 1 + @counter ||= 0 + @counter += 1 end end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 8cc3c1473e2..3137dd32f93 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -99,7 +99,7 @@ class Namespace < ActiveRecord::Base path = "blank" if path.blank? uniquify = Uniquify.new - uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) }) + uniquify.string(path) { |s| Namespace.find_by_path_or_name(s) } end end diff --git a/app/models/user.rb b/app/models/user.rb index 7207cdc4581..4bb9dc0c59f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -346,43 +346,7 @@ class User < ActiveRecord::Base # Return (create if necessary) the ghost user. The ghost user # owns records previously belonging to deleted users. def ghost - ghost_user = User.find_by_ghost(true) - - ghost_user || - begin - # Since we only want a single ghost user in an instance, we use an - # 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) - # and the time we acquired the lock. - ghost_user = User.find_by_ghost(true) - return ghost_user if ghost_user.present? - - uniquify = Uniquify.new - - username = uniquify.string("ghost", -> (s) { User.find_by_username(s) }) - - email = uniquify.string( - -> (n) { "ghost#{n}@example.com" }, - -> (s) { User.find_by_email(s) } - ) - - User.create( - username: username, password: Devise.friendly_token, - email: email, name: "Ghost User", state: :blocked, ghost: true - ) - ensure - Gitlab::ExclusiveLease.cancel(lease_key, uuid) - end + User.find_by_ghost(true) || create_ghost_user end end @@ -1052,4 +1016,38 @@ class User < ActiveRecord::Base super end end + + def self.create_ghost_user + # Since we only want a single ghost user in an instance, we use an + # 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) + # and the time we acquired the lock. + ghost_user = User.find_by_ghost(true) + return ghost_user if ghost_user.present? + + uniquify = Uniquify.new + + username = uniquify.string("ghost") { |s| User.find_by_username(s) } + + email = uniquify.string(-> (n) { "ghost#{n}@example.com" }) do |s| + User.find_by_email(s) + end + + User.create( + username: username, password: Devise.friendly_token, + email: email, name: "Ghost User", state: :blocked, ghost: true + ) + ensure + Gitlab::ExclusiveLease.cancel(lease_key, uuid) + end end diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index bf1e49734cc..523279944ae 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -37,13 +37,18 @@ module Users end private - + def move_issues_to_ghost_user(user) + # Block the user before moving issues to prevent a data race. + # If the user creates an issue after `move_issues_to_ghost_user` + # runs and before the user is destroyed, the destroy will fail with + # an exception. We block the user so that issues can't be created + # after `move_issues_to_ghost_user` runs and before the destroy happens. + user.block + ghost_user = User.ghost - Issue.transaction do - user.issues.update_all(author_id: ghost_user.id) - end + user.issues.update_all(author_id: ghost_user.id) user.reload end diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb index 4e06a1f4c31..83187d732e4 100644 --- a/spec/models/concerns/uniquify_spec.rb +++ b/spec/models/concerns/uniquify_spec.rb @@ -1,38 +1,31 @@ require 'spec_helper' describe Uniquify, models: true do + let(:uniquify) { described_class.new } + describe "#string" do it 'returns the given string if it does not exist' do - uniquify = Uniquify.new - - result = uniquify.string('test_string', -> (s) { false }) + 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' }) + result = uniquify.string('test_string') { |s| 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' }) + result = uniquify.string('test_string') { |s| 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' } - ) + result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s| + s == 'test__string' + end expect(result).to eq('test_1_string') end diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb index 4f0834064e5..922e82445d0 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_spec.rb @@ -47,6 +47,10 @@ describe Users::DestroyService, services: true do expect(migrated_issue.author).to eq(User.ghost) end + + it 'blocks the user before migrating issues to the "Ghost User' do + expect(user).to be_blocked + end end context "for an issue the user was assigned to" do