diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2017-02-06 17:18:27 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2017-02-24 16:50:19 +0530 |
commit | 53c34c7436112d7cac9c3887ada1d5ae630a206c (patch) | |
tree | 6a3262eef1760c3aa3d88ef9bc57b3f88bb85ceb | |
parent | ca16c3734b7b89f71bdc9e1c18152aa1599c4f89 (diff) | |
download | gitlab-ce-53c34c7436112d7cac9c3887ada1d5ae630a206c.tar.gz |
Implement review comments from @DouweM and @nick.thomas.
1. Use an advisory lock to guarantee the absence of concurrency in `User.ghost`,
to prevent data races from creating more than one ghost, or preventing the
creation of ghost users by causing validation errors.
2. Use `update_all` instead of updating issues one-by-one.
-rw-r--r-- | app/models/user.rb | 13 | ||||
-rw-r--r-- | app/services/users/destroy_service.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/database/advisory_locking.rb | 70 |
3 files changed, 85 insertions, 2 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 13529c9997a..e4adcd6cde9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -351,6 +351,17 @@ 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 + + # 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.with_state(:ghost).first + return ghost_user if ghost_user.present? + uniquify = Uniquify.new username = uniquify.string("ghost", -> (s) { User.find_by_username(s) }) @@ -364,6 +375,8 @@ class User < ActiveRecord::Base username: username, password: Devise.friendly_token, email: email, name: "Ghost User", state: :ghost ) + ensure + advisory_lock.unlock end end end diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index d88b81c04e2..bf1e49734cc 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -37,12 +37,12 @@ module Users end private - + def move_issues_to_ghost_user(user) ghost_user = User.ghost Issue.transaction do - user.issues.each { |issue| issue.update!(author: ghost_user) } + user.issues.update_all(author_id: ghost_user.id) end user.reload diff --git a/lib/gitlab/database/advisory_locking.rb b/lib/gitlab/database/advisory_locking.rb new file mode 100644 index 00000000000..950898288be --- /dev/null +++ b/lib/gitlab/database/advisory_locking.rb @@ -0,0 +1,70 @@ +# 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 |