diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2017-02-16 10:05:10 +0300 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2017-02-24 14:20:20 +0300 |
commit | f2ed82fa8486875660b80dd061827ac8b86d00b6 (patch) | |
tree | 07b0221a225f32de7a3063a4bee2ff97463f9c54 /app | |
parent | 3bd2a98f64a12a2e23a6b9ce77427a9c2033a6bf (diff) |
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
Diffstat (limited to 'app')
-rw-r--r-- | app/models/concerns/uniquify.rb | 9 | ||||
-rw-r--r-- | app/models/namespace.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 72 | ||||
-rw-r--r-- | app/services/users/destroy_service.rb | 13 |
4 files changed, 50 insertions, 46 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 |