Commit 27a8cc7b authored by Sean McGivern's avatar Sean McGivern

Merge branch 'sidekiq-transaction-ee' into 'master'

Forbid Sidekiq scheduling in transactions [EE]

See merge request !2024
parents 7a048641 f09429b2
......@@ -49,8 +49,8 @@ module Ci
before_destroy { unscoped_project }
after_create :execute_hooks
after_save :update_project_statistics, if: :artifacts_size_changed?
after_destroy :update_project_statistics
after_commit :update_project_statistics_after_save, on: [:create, :update]
after_commit :update_project_statistics, on: :destroy
class << self
# This is needed for url_for to work,
......@@ -530,5 +530,11 @@ module Ci
ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
end
def update_project_statistics_after_save
if previous_changes.include?('artifacts_size')
update_project_statistics
end
end
end
end
......@@ -18,7 +18,7 @@ class CommitStatus < ActiveRecord::Base
validates :name, presence: true
alias_attribute :author, :user
scope :failed_but_allowed, -> do
where(allow_failure: true, status: [:failed, :canceled])
end
......@@ -83,14 +83,15 @@ class CommitStatus < ActiveRecord::Base
next if transition.loopback?
commit_status.run_after_commit do
pipeline.try do |pipeline|
if pipeline
if complete? || manual?
PipelineProcessWorker.perform_async(pipeline.id)
else
PipelineUpdateWorker.perform_async(pipeline.id)
end
ExpireJobCacheWorker.perform_async(commit_status.id)
end
ExpireJobCacheWorker.perform_async(commit_status.id)
end
end
......
......@@ -5,8 +5,7 @@ class IssueAssignee < ActiveRecord::Base
belongs_to :assignee, class_name: "User", foreign_key: :user_id
# EE-specific
after_create :update_elasticsearch_index
after_destroy :update_elasticsearch_index
after_commit :update_elasticsearch_index, on: [:create, :destroy]
# EE-specific
def update_elasticsearch_index
......
require 'digest/md5'
class Key < ActiveRecord::Base
include AfterCommitQueue
include Sortable
LAST_USED_AT_REFRESH_TIME = 1.day.to_i
......@@ -27,10 +26,10 @@ class Key < ActiveRecord::Base
delegate :name, :email, to: :user, prefix: true
after_create :add_to_shell
after_create :notify_user
after_commit :add_to_shell, on: :create
after_commit :notify_user, on: :create
after_create :post_create_hook
after_destroy :remove_from_shell
after_commit :remove_from_shell, on: :destroy
after_destroy :post_destroy_hook
def key=(value)
......@@ -95,6 +94,6 @@ class Key < ActiveRecord::Base
end
def notify_user
run_after_commit { NotificationService.new.new_key(self) }
NotificationService.new.new_key(self)
end
end
......@@ -6,8 +6,7 @@ class LfsObjectsProject < ActiveRecord::Base
validates :lfs_object_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
validates :project_id, presence: true
after_create :update_project_statistics
after_destroy :update_project_statistics
after_commit :update_project_statistics, on: [:create, :destroy]
private
......
......@@ -7,6 +7,7 @@ class Namespace < ActiveRecord::Base
include Gitlab::ShellAdapter
include Gitlab::CurrentSettings
include Routable
include AfterCommitQueue
# Prevent users from creating unreasonably deep level of nesting.
# The number 20 was taken based on maximum nesting level of
......@@ -253,7 +254,9 @@ class Namespace < ActiveRecord::Base
# Remove namespace directroy async with delay so
# GitLab has time to remove all projects first
GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
run_after_commit do
GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
end
end
end
......
......@@ -516,7 +516,9 @@ class Project < ActiveRecord::Base
end
def reset_cache_and_import_attrs
ProjectCacheWorker.perform_async(self.id)
run_after_commit do
ProjectCacheWorker.perform_async(self.id)
end
self.import_data&.destroy unless mirror?
end
......
......@@ -7,11 +7,9 @@ module Projects
DELETED_FLAG = '+deleted'.freeze
def async_execute
project.transaction do
project.update_attribute(:pending_delete, true)
job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
end
project.update_attribute(:pending_delete, true)
job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
end
def execute
......@@ -95,7 +93,11 @@ module Projects
if gitlab_shell.mv_repository(project.repository_storage_path, path, new_path)
log_info("Repository \"#{path}\" moved to \"#{new_path}\"")
GitlabShellWorker.perform_in(5.minutes, :remove_repository, project.repository_storage_path, new_path)
project.run_after_commit do
# self is now project
GitlabShellWorker.perform_in(5.minutes, :remove_repository, self.repository_storage_path, new_path)
end
else
false
end
......
module Sidekiq
module Worker
mattr_accessor :skip_transaction_check
self.skip_transaction_check = false
def self.skipping_transaction_check(&block)
skip_transaction_check = self.skip_transaction_check
self.skip_transaction_check = true
yield
ensure
self.skip_transaction_check = skip_transaction_check
end
module ClassMethods
module NoSchedulingFromTransactions
NESTING = ::Rails.env.test? ? 1 : 0
%i(perform_async perform_at perform_in).each do |name|
define_method(name) do |*args|
return super(*args) if Sidekiq::Worker.skip_transaction_check
return super(*args) unless ActiveRecord::Base.connection.open_transactions > NESTING
raise <<-MSG.strip_heredoc
`#{self}.#{name}` cannot be called inside a transaction as this can lead to
race conditions when the worker runs before the transaction is committed and
tries to access a model that has not been saved yet.
Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.
MSG
end
end
end
prepend NoSchedulingFromTransactions
end
end
end
module ActiveRecord
class Base
module SkipTransactionCheckAfterCommit
def committed!(*)
Sidekiq::Worker.skipping_transaction_check { super }
end
end
prepend SkipTransactionCheckAfterCommit
end
end
require './spec/support/sidekiq'
# Creating keys runs a gitlab-shell worker. Since we may not have the right
# gitlab-shell path set (yet) we need to disable this for these fixtures.
Sidekiq::Testing.disable! do
Gitlab::Seeder.quiet do
# We want to run `add_to_shell` immediately instead of after the commit, so
# that it falls under `Sidekiq::Testing.disable!`.
Key.skip_callback(:commit, :after, :add_to_shell)
User.first(10).each do |user|
key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
user.keys.create(
key = user.keys.create(
title: "Sample key #{user.id}",
key: key
)
Sidekiq::Worker.skipping_transaction_check do
key.add_to_shell
end
print '.'
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