Commit 3cc5e486 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'sidekiq-transaction' into 'master'

Forbid Sidekiq scheduling in transactions

Closes #27233

See merge request !9376
parents 3b39cf4e 103b5bf6
No related merge requests found
...@@ -47,8 +47,8 @@ module Ci ...@@ -47,8 +47,8 @@ module Ci
before_destroy { unscoped_project } before_destroy { unscoped_project }
after_create :execute_hooks after_create :execute_hooks
after_save :update_project_statistics, if: :artifacts_size_changed? after_commit :update_project_statistics_after_save, on: [:create, :update]
after_destroy :update_project_statistics after_commit :update_project_statistics, on: :destroy
class << self class << self
# This is needed for url_for to work, # This is needed for url_for to work,
...@@ -512,5 +512,11 @@ module Ci ...@@ -512,5 +512,11 @@ module Ci
ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
end end
def update_project_statistics_after_save
if previous_changes.include?('artifacts_size')
update_project_statistics
end
end
end end
end end
...@@ -18,7 +18,7 @@ class CommitStatus < ActiveRecord::Base ...@@ -18,7 +18,7 @@ class CommitStatus < ActiveRecord::Base
validates :name, presence: true validates :name, presence: true
alias_attribute :author, :user alias_attribute :author, :user
scope :failed_but_allowed, -> do scope :failed_but_allowed, -> do
where(allow_failure: true, status: [:failed, :canceled]) where(allow_failure: true, status: [:failed, :canceled])
end end
...@@ -83,14 +83,15 @@ class CommitStatus < ActiveRecord::Base ...@@ -83,14 +83,15 @@ class CommitStatus < ActiveRecord::Base
next if transition.loopback? next if transition.loopback?
commit_status.run_after_commit do commit_status.run_after_commit do
pipeline.try do |pipeline| if pipeline
if complete? || manual? if complete? || manual?
PipelineProcessWorker.perform_async(pipeline.id) PipelineProcessWorker.perform_async(pipeline.id)
else else
PipelineUpdateWorker.perform_async(pipeline.id) PipelineUpdateWorker.perform_async(pipeline.id)
end end
ExpireJobCacheWorker.perform_async(commit_status.id)
end end
ExpireJobCacheWorker.perform_async(commit_status.id)
end end
end end
......
require 'digest/md5' require 'digest/md5'
class Key < ActiveRecord::Base class Key < ActiveRecord::Base
include AfterCommitQueue
include Sortable include Sortable
LAST_USED_AT_REFRESH_TIME = 1.day.to_i LAST_USED_AT_REFRESH_TIME = 1.day.to_i
...@@ -25,10 +24,10 @@ class Key < ActiveRecord::Base ...@@ -25,10 +24,10 @@ class Key < ActiveRecord::Base
delegate :name, :email, to: :user, prefix: true delegate :name, :email, to: :user, prefix: true
after_create :add_to_shell after_commit :add_to_shell, on: :create
after_create :notify_user after_commit :notify_user, on: :create
after_create :post_create_hook after_create :post_create_hook
after_destroy :remove_from_shell after_commit :remove_from_shell, on: :destroy
after_destroy :post_destroy_hook after_destroy :post_destroy_hook
def key=(value) def key=(value)
...@@ -93,6 +92,6 @@ class Key < ActiveRecord::Base ...@@ -93,6 +92,6 @@ class Key < ActiveRecord::Base
end end
def notify_user def notify_user
run_after_commit { NotificationService.new.new_key(self) } NotificationService.new.new_key(self)
end end
end end
...@@ -6,8 +6,7 @@ class LfsObjectsProject < ActiveRecord::Base ...@@ -6,8 +6,7 @@ class LfsObjectsProject < ActiveRecord::Base
validates :lfs_object_id, uniqueness: { scope: [:project_id], message: "already exists in project" } validates :lfs_object_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
validates :project_id, presence: true validates :project_id, presence: true
after_create :update_project_statistics after_commit :update_project_statistics, on: [:create, :destroy]
after_destroy :update_project_statistics
private private
......
...@@ -6,6 +6,7 @@ class Namespace < ActiveRecord::Base ...@@ -6,6 +6,7 @@ class Namespace < ActiveRecord::Base
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include Routable include Routable
include AfterCommitQueue
# Prevent users from creating unreasonably deep level of nesting. # Prevent users from creating unreasonably deep level of nesting.
# The number 20 was taken based on maximum nesting level of # The number 20 was taken based on maximum nesting level of
...@@ -242,7 +243,9 @@ class Namespace < ActiveRecord::Base ...@@ -242,7 +243,9 @@ class Namespace < ActiveRecord::Base
# Remove namespace directroy async with delay so # Remove namespace directroy async with delay so
# GitLab has time to remove all projects first # 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
end end
......
...@@ -471,7 +471,9 @@ class Project < ActiveRecord::Base ...@@ -471,7 +471,9 @@ class Project < ActiveRecord::Base
end end
def reset_cache_and_import_attrs 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 self.import_data&.destroy
end end
......
...@@ -7,11 +7,9 @@ module Projects ...@@ -7,11 +7,9 @@ module Projects
DELETED_FLAG = '+deleted'.freeze DELETED_FLAG = '+deleted'.freeze
def async_execute def async_execute
project.transaction do project.update_attribute(:pending_delete, true)
project.update_attribute(:pending_delete, true) job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
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}")
Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
end
end end
def execute def execute
...@@ -62,7 +60,11 @@ module Projects ...@@ -62,7 +60,11 @@ module Projects
if gitlab_shell.mv_repository(project.repository_storage_path, path, new_path) if gitlab_shell.mv_repository(project.repository_storage_path, path, new_path)
log_info("Repository \"#{path}\" moved to \"#{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 else
false false
end 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' require './spec/support/sidekiq'
# Creating keys runs a gitlab-shell worker. Since we may not have the right # 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. # gitlab-shell path set (yet) we need to disable this for these fixtures.
Sidekiq::Testing.disable! do Sidekiq::Testing.disable! do
Gitlab::Seeder.quiet 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| User.first(10).each do |user|
key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=" key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
user.keys.create( key = user.keys.create(
title: "Sample key #{user.id}", title: "Sample key #{user.id}",
key: key key: key
) )
Sidekiq::Worker.skipping_transaction_check do
key.add_to_shell
end
print '.' print '.'
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