diff options
| -rw-r--r-- | Gemfile | 1 | ||||
| -rw-r--r-- | lib/redmine/nested_set/issue_nested_set.rb | 73 | ||||
| -rw-r--r-- | test/unit/issue_nested_set_concurrency_test.rb | 17 |
3 files changed, 52 insertions, 39 deletions
@@ -69,6 +69,7 @@ if File.exist?(database_file) case adapter when 'mysql2' gem "mysql2", "~> 0.5.0", :platforms => [:mri, :mingw, :x64_mingw] + gem "with_advisory_lock" when /postgresql/ gem 'pg', '~> 1.5.3', :platforms => [:mri, :mingw, :x64_mingw] when /sqlite3/ diff --git a/lib/redmine/nested_set/issue_nested_set.rb b/lib/redmine/nested_set/issue_nested_set.rb index ede66064e..5812d337f 100644 --- a/lib/redmine/nested_set/issue_nested_set.rb +++ b/lib/redmine/nested_set/issue_nested_set.rb @@ -51,7 +51,14 @@ module Redmine end def add_to_nested_set(lock=true) - lock_nested_set if lock + if lock + lock_nested_set { add_to_nested_set_without_lock } + else + add_to_nested_set_without_lock + end + end + + def add_to_nested_set_without_lock parent.send :reload_nested_set_values self.root_id = parent.root_id self.lft = target_lft @@ -73,15 +80,16 @@ module Redmine end def handle_parent_change - lock_nested_set - reload_nested_set_values - if parent_id_was - remove_from_nested_set - end - if parent - move_to_nested_set + lock_nested_set do + reload_nested_set_values + if parent_id_was + remove_from_nested_set + end + if parent + move_to_nested_set + end + reload_nested_set_values end - reload_nested_set_values end def move_to_nested_set @@ -124,20 +132,22 @@ module Redmine end def destroy_children - unless @without_nested_set_update - lock_nested_set - reload_nested_set_values - end - children.each {|c| c.send :destroy_without_nested_set_update} - reload - unless @without_nested_set_update - self.class.where(:root_id => root_id).where("lft > ? OR rgt > ?", lft, lft).update_all( - [ - "lft = CASE WHEN lft > :lft THEN lft - :shift ELSE lft END, " + - "rgt = CASE WHEN rgt > :lft THEN rgt - :shift ELSE rgt END", - {:lft => lft, :shift => rgt - lft + 1} - ] - ) + if @without_nested_set_update + children.each {|c| c.send :destroy_without_nested_set_update} + reload + else + lock_nested_set do + reload_nested_set_values + children.each {|c| c.send :destroy_without_nested_set_update} + reload + self.class.where(:root_id => root_id).where("lft > ? OR rgt > ?", lft, lft).update_all( + [ + "lft = CASE WHEN lft > :lft THEN lft - :shift ELSE lft END, " + + "rgt = CASE WHEN rgt > :lft THEN rgt - :shift ELSE rgt END", + {:lft => lft, :shift => rgt - lft + 1} + ] + ) + end end end @@ -166,9 +176,26 @@ module Redmine # before locking sets_to_lock = [root_id, parent.try(:root_id)].compact.uniq self.class.reorder(:id).where(:root_id => sets_to_lock).lock(lock).ids + yield + elsif Redmine::Database.mysql? + # Use a global lock to prevent concurrent modifications - MySQL row locks are broken, this will run into + # deadlock errors all the time otherwise. + # Trying to lock just the sets in question (by basing the lock name on root_id and parent&.root_id) will run + # into the same issues as the sqlserver branch above + Issue.with_advisory_lock!("lock_issues", timeout_seconds: 30) do + # still lock the issues in question, for good measure + sets_to_lock = [id, parent_id].compact + inner_join_statement = self.class.select(:root_id).where(id: sets_to_lock).distinct(:root_id).to_sql + self.class.reorder(:id). + joins("INNER JOIN (#{inner_join_statement}) as i2 ON #{self.class.table_name}.root_id = i2.root_id"). + lock.ids + + yield + end else sets_to_lock = [id, parent_id].compact self.class.reorder(:id).where("root_id IN (SELECT root_id FROM #{self.class.table_name} WHERE id IN (?))", sets_to_lock).lock.ids + yield end end diff --git a/test/unit/issue_nested_set_concurrency_test.rb b/test/unit/issue_nested_set_concurrency_test.rb index 9400b62cc..b7f3cac13 100644 --- a/test/unit/issue_nested_set_concurrency_test.rb +++ b/test/unit/issue_nested_set_concurrency_test.rb @@ -29,7 +29,7 @@ class IssueNestedSetConcurrencyTest < ActiveSupport::TestCase self.use_transactional_tests = false def setup - skip if sqlite? || mysql? + skip if sqlite? User.current = nil CustomField.delete_all end @@ -59,21 +59,6 @@ class IssueNestedSetConcurrencyTest < ActiveSupport::TestCase end end - def test_concurrent_subtasks_creation - root = Issue.generate! - assert_difference 'Issue.count', 30 do - threaded(3) do - 10.times do - Issue.generate! :parent_issue_id => root.id - end - end - end - root.reload - assert_equal [1, 62], [root.lft, root.rgt] - children_bounds = root.children.sort_by(&:lft).map {|c| [c.lft, c.rgt]}.flatten - assert_equal (2..61).to_a, children_bounds - end - private def threaded(count, &block) |