From b0ee22609a89572d6e3f98eebccf9fb2335dd939 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Fri, 5 May 2017 14:31:33 -0700
Subject: [PATCH] Reduce risk of deadlocks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We’ve seen a deadlock in CI here https://gitlab.com/mkozono/gitlab-ce/builds/15644492#down-build-trace. This commit should not fix that particular failure, but perhaps it will avoid others.
* Don’t call delete_conflicting_redirects after update if the path wasn’t changed
* Rename descendants without using recursion again, so we can run delete_conflicting_redirects exactly once.
---
app/models/route.rb | 24 +++++++++++++++++-------
spec/models/route_spec.rb | 15 +--------------
2 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/app/models/route.rb b/app/models/route.rb
index b34cce9077a..12a7fa3d01b 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,19 +8,19 @@ class Route < ActiveRecord::Base
presence: true,
uniqueness: { case_sensitive: false }
- after_save :delete_conflicting_redirects
+ after_create :delete_conflicting_redirects
+ after_update :delete_conflicting_redirects, if: :path_changed?
after_update :create_redirect_for_old_path
- after_update :rename_direct_descendant_routes
+ after_update :rename_descendants
scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") }
- scope :direct_descendant_routes, -> (path) { where('routes.path LIKE ? AND routes.path NOT LIKE ?', "#{sanitize_sql_like(path)}/%", "#{sanitize_sql_like(path)}/%/%") }
- def rename_direct_descendant_routes
+ def rename_descendants
return unless path_changed? || name_changed?
- direct_descendant_routes = self.class.direct_descendant_routes(path_was)
+ descendant_routes = self.class.inside_path(path_was)
- direct_descendant_routes.each do |route|
+ descendant_routes.each do |route|
attributes = {}
if path_changed? && route.path.present?
@@ -31,7 +31,17 @@ class Route < ActiveRecord::Base
attributes[:name] = route.name.sub(name_was, name)
end
- route.update(attributes) unless attributes.empty?
+ if attributes.present?
+ old_path = route.path
+
+ # Callbacks must be run manually
+ route.update_columns(attributes)
+
+ # We are not calling route.delete_conflicting_redirects here, in hopes
+ # of avoiding deadlocks. The parent (self, in this method) already
+ # called it, which deletes conflicts for all descendants.
+ route.create_redirect(old_path) if attributes[:path]
+ end
end
end
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index 1aeddcef982..c1fe1b06c52 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -50,20 +50,7 @@ describe Route, models: true do
end
end
- describe '.direct_descendant_routes' do
- let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
- let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
- let!(:another_group) { create(:group, path: 'other') }
- let!(:similar_group) { create(:group, path: 'gitllab') }
- let!(:another_group_nested) { create(:group, path: 'another', name: 'another', parent: similar_group) }
-
- it 'returns correct routes' do
- expect(Route.direct_descendant_routes('git_lab')).to match_array([nested_group.route])
- expect(Route.direct_descendant_routes('git_lab/test')).to match_array([deep_nested_group.route])
- end
- end
-
- describe '#rename_direct_descendant_routes' do
+ describe '#rename_descendants' do
let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
let!(:similar_group) { create(:group, path: 'gitlab-org', name: 'gitlab-org') }
--
GitLab