Merged r22458, r22459, r22460, r22461, r22462 and r22464 from trunk to 5.1-stable (#39437).

git-svn-id: https://svn.redmine.org/redmine/branches/5.1-stable@22467 e93f8b46-1217-0410-a6f0-8f06a7374b81

Gemfile

@@ -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/

app/models/issue.rb

@@ -119,13 +119,14 @@ class Issue < ActiveRecord::Base
   after_save :reschedule_following_issues, :update_nested_set_attributes,
              :update_parent_attributes, :delete_selected_attachments, :create_journal
   # Should be after_create but would be called before previous after_save callbacks
-  after_save :after_create_from_copy, :create_parent_issue_journal
-  after_destroy :update_parent_attributes, :create_parent_issue_journal
+  after_save :after_create_from_copy
+  after_destroy :update_parent_attributes
   # add_auto_watcher needs to run before sending notifications, thus it needs
   # to be added after send_notification (after_ callbacks are run in inverse order)
   # https://api.rubyonrails.org/v5.2.3/classes/ActiveSupport/Callbacks/ClassMethods.html#method-i-set_callback
   after_create_commit :send_notification
   after_create_commit :add_auto_watcher
+  after_commit :create_parent_issue_journal
 
   # Returns a SQL conditions string used to find all issues visible by the specified user
   def self.visible_condition(user, options={})
@@ -2027,15 +2028,24 @@ class Issue < ActiveRecord::Base
         [nil, parent_id]
       end
 
-    if old_parent_id.present? && old_parent_issue = Issue.visible.find_by_id(old_parent_id)
-      old_parent_issue.init_journal(User.current)
-      old_parent_issue.current_journal.__send__(:add_attribute_detail, 'child_id', child_id, nil)
-      old_parent_issue.save
+    if old_parent_id.present?
+      Issue.transaction do
+        if old_parent_issue = Issue.visible.lock.find_by_id(old_parent_id)
+          old_parent_issue.init_journal(User.current)
+          old_parent_issue.current_journal.__send__(:add_attribute_detail, 'child_id', child_id, nil)
+          old_parent_issue.save
+        end
+      end
     end
-    if new_parent_id.present? && new_parent_issue = Issue.visible.find_by_id(new_parent_id)
-      new_parent_issue.init_journal(User.current)
-      new_parent_issue.current_journal.__send__(:add_attribute_detail, 'child_id', nil, child_id)
-      new_parent_issue.save
+
+    if new_parent_id.present?
+      Issue.transaction do
+        if new_parent_issue = Issue.visible.lock.find_by_id(new_parent_id)
+          new_parent_issue.init_journal(User.current)
+          new_parent_issue.current_journal.__send__(:add_attribute_detail, 'child_id', nil, child_id)
+          new_parent_issue.save
+        end
+      end
     end
   end
 

lib/redmine/database.rb

@@ -61,6 +61,10 @@ module Redmine
         /mysql/i.match?(ActiveRecord::Base.connection.adapter_name)
       end
 
+      def mysql_version
+        mysql? ? ActiveRecord::Base.connection.select_value("SELECT VERSION()") : nil
+      end
+
       # Returns a SQL statement for case/accent (if possible) insensitive match
       def like(left, right, options={})
         neg = (options[:match] == false ? 'NOT ' : '')

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
+        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
-        if parent
-          move_to_nested_set
-        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
 

test/test_helper.rb

@@ -203,6 +203,10 @@ class ActiveSupport::TestCase
     Redmine::Database.mysql?
   end
 
+  def mysql8?
+    Gem::Version.new(Redmine::Database.mysql_version) >= Gem::Version.new('8.0.0')
+  end
+
   def postgresql?
     Redmine::Database.postgresql?
   end

test/unit/issue_nested_set_concurrency_test.rb

@@ -29,7 +29,12 @@ class IssueNestedSetConcurrencyTest < ActiveSupport::TestCase
   self.use_transactional_tests = false
 
   def setup
-    skip if sqlite? || mysql?
+    skip if sqlite?
+    if mysql?
+      connection = ActiveRecord::Base.connection_db_config.configuration_hash.deep_dup
+      connection[:variables] = mysql8? ? { transaction_isolation: "READ-COMMITTED" } : { tx_isolation: "READ-COMMITTED" }
+      ActiveRecord::Base.establish_connection connection
+    end
     User.current = nil
     CustomField.delete_all
   end
@@ -74,6 +79,49 @@ class IssueNestedSetConcurrencyTest < ActiveSupport::TestCase
     assert_equal (2..61).to_a, children_bounds
   end
 
+  def test_concurrent_subtask_removal
+    with_settings :notified_events => [] do
+      root = Issue.generate!
+      60.times do
+        Issue.generate! :parent_issue_id => root.id
+      end
+      # pick 40 random subtask ids
+      child_ids = Issue.where(root_id: root.id, parent_id: root.id).pluck(:id)
+      ids_to_remove = child_ids.sample(40).shuffle
+      ids_to_keep = child_ids - ids_to_remove
+      # remove these from the set, using four parallel threads
+      threads = []
+      ids_to_remove.each_slice(10) do |ids|
+        threads << Thread.new do
+          ActiveRecord::Base.connection_pool.with_connection do
+            begin
+              ids.each do |id|
+                Issue.find(id).update(parent_id: nil)
+              end
+            rescue => e
+              Thread.current[:exception] = e.message
+            end
+          end
+        end
+      end
+      threads.each do |thread|
+        thread.join
+        assert_nil thread[:exception]
+      end
+      assert_equal 20, Issue.where(parent_id: root.id).count
+      Issue.where(id: ids_to_remove).each do |issue|
+        assert_nil issue.parent_id
+        assert_equal issue.id, issue.root_id
+        assert_equal 1, issue.lft
+        assert_equal 2, issue.rgt
+      end
+      root.reload
+      assert_equal [1, 42], [root.lft, root.rgt]
+      children_bounds = root.children.sort_by(&:lft).map {|c| [c.lft, c.rgt]}.flatten
+      assert_equal (2..41).to_a, children_bounds
+    end
+  end
+
   private
 
   def threaded(count, &block)