Commit 53c34c74 authored by Timothy Andrew's avatar Timothy Andrew

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.
parent ca16c373
...@@ -351,6 +351,17 @@ class User < ActiveRecord::Base ...@@ -351,6 +351,17 @@ class User < ActiveRecord::Base
ghost_user || ghost_user ||
begin 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 uniquify = Uniquify.new
username = uniquify.string("ghost", -> (s) { User.find_by_username(s) }) username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
...@@ -364,6 +375,8 @@ class User < ActiveRecord::Base ...@@ -364,6 +375,8 @@ class User < ActiveRecord::Base
username: username, password: Devise.friendly_token, username: username, password: Devise.friendly_token,
email: email, name: "Ghost User", state: :ghost email: email, name: "Ghost User", state: :ghost
) )
ensure
advisory_lock.unlock
end end
end end
end end
......
...@@ -37,12 +37,12 @@ module Users ...@@ -37,12 +37,12 @@ module Users
end end
private private
def move_issues_to_ghost_user(user) def move_issues_to_ghost_user(user)
ghost_user = User.ghost ghost_user = User.ghost
Issue.transaction do Issue.transaction do
user.issues.each { |issue| issue.update!(author: ghost_user) } user.issues.update_all(author_id: ghost_user.id)
end end
user.reload user.reload
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment