diff --git a/lib/closure_tree/finders.rb b/lib/closure_tree/finders.rb index c95b5c1..e66ead0 100644 --- a/lib/closure_tree/finders.rb +++ b/lib/closure_tree/finders.rb @@ -17,7 +17,7 @@ def find_or_create_by_path(path, attributes = {}) return found if found attrs = subpath.shift - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # shenanigans because children.create is bound to the superclass # (in the case of polymorphism): child = self.children.where(attrs).first || begin @@ -159,7 +159,7 @@ def find_or_create_by_path(path, attributes = {}) attr_path = _ct.build_ancestry_attr_path(path, attributes) find_by_path(attr_path) || begin root_attrs = attr_path.shift - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # shenanigans because find_or_create can't infer that we want the same class as this: # Note that roots will already be constrained to this subclass (in the case of polymorphism): root = roots.where(root_attrs).first || _ct.create!(self, root_attrs) diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index 4b19f78..023eb20 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -12,6 +12,7 @@ def has_closure_tree(options = {}) :numeric_order, :touch, :with_advisory_lock, + :advisory_lock_timeout_seconds, :order_belong_to ) diff --git a/lib/closure_tree/hierarchy_maintenance.rb b/lib/closure_tree/hierarchy_maintenance.rb index 3023b24..0484570 100644 --- a/lib/closure_tree/hierarchy_maintenance.rb +++ b/lib/closure_tree/hierarchy_maintenance.rb @@ -53,7 +53,7 @@ def _ct_after_save end def _ct_before_destroy - _ct.with_advisory_lock do + _ct.with_advisory_lock! do delete_hierarchy_references if _ct.options[:dependent] == :nullify self.class.find(self.id).children.find_each { |c| c.rebuild! } @@ -63,7 +63,7 @@ def _ct_before_destroy end def rebuild!(called_by_rebuild = false) - _ct.with_advisory_lock do + _ct.with_advisory_lock! do delete_hierarchy_references unless (defined? @was_new_record) && @was_new_record hierarchy_class.create!(:ancestor => self, :descendant => self, :generations => 0) unless root? @@ -89,7 +89,7 @@ def rebuild!(called_by_rebuild = false) end def delete_hierarchy_references - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # The crazy double-wrapped sub-subselect works around MySQL's limitation of subselects on the same table that is being mutated. # It shouldn't affect performance of postgresql. # See http://dev.mysql.com/doc/refman/5.0/en/subquery-errors.html @@ -111,7 +111,7 @@ module ClassMethods # Rebuilds the hierarchy table based on the parent_id column in the database. # Note that the hierarchy table will be truncated. def rebuild! - _ct.with_advisory_lock do + _ct.with_advisory_lock! do cleanup! roots.find_each { |n| n.send(:rebuild!) } # roots just uses the parent_id column, so this is safe. end diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index e62837f..6c36b8f 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -125,7 +125,7 @@ def add_sibling(sibling, add_after = true) # Make sure self isn't dirty, because we're going to call reload: save - _ct.with_advisory_lock do + _ct.with_advisory_lock! do prior_sibling_parent = sibling.parent reorder_from_value = if prior_sibling_parent == self.parent [self.order_value, sibling.order_value].compact.min diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 04cf06b..2874c44 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -23,6 +23,7 @@ def initialize(model_class, options) :dependent => :nullify, # or :destroy or :delete_all -- see the README :name_column => 'name', :with_advisory_lock => true, + :advisory_lock_timeout_seconds => 15, :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' @@ -108,9 +109,9 @@ def where_eq(column_name, value) end end - def with_advisory_lock(&block) + def with_advisory_lock!(&block) if options[:with_advisory_lock] - model_class.with_advisory_lock(advisory_lock_name) do + model_class.with_advisory_lock!(advisory_lock_name, advisory_lock_options) do transaction { yield } end else diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index 2741c29..cf28553 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -8,6 +8,10 @@ def advisory_lock_name Digest::SHA1.hexdigest("ClosureTree::#{base_class.name}")[0..32] end + def advisory_lock_options + { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact + end + def quoted_table_name connection.quote_table_name(table_name) end diff --git a/spec/closure_tree/parallel_spec.rb b/spec/closure_tree/parallel_spec.rb index 4ec3a61..7cdf995 100644 --- a/spec/closure_tree/parallel_spec.rb +++ b/spec/closure_tree/parallel_spec.rb @@ -103,7 +103,7 @@ def run_workers(worker_class = FindOrCreateWorker) it 'creates dupe roots without advisory locks' do # disable with_advisory_lock: - allow(Tag).to receive(:with_advisory_lock) { |_lock_name, &block| block.call } + allow(Tag).to receive(:with_advisory_lock!) { |_lock_name, &block| block.call } run_workers # duplication from at least one iteration: expect(Tag.where(name: @names).size).to be > @iterations diff --git a/spec/closure_tree/support_spec.rb b/spec/closure_tree/support_spec.rb index 1deb27b..d63a3db 100644 --- a/spec/closure_tree/support_spec.rb +++ b/spec/closure_tree/support_spec.rb @@ -1,14 +1,38 @@ require 'spec_helper' RSpec.describe ClosureTree::Support do - let(:sut) { Tag._ct } + let(:model) { Tag } + let(:options) { {} } + let(:sut) { described_class.new(model, options) } + it 'passes through table names without prefix and suffix' do expected = 'some_random_table_name' expect(sut.remove_prefix_and_suffix(expected)).to eq(expected) end + it 'extracts through table name with prefix and suffix' do expected = 'some_random_table_name' tn = ActiveRecord::Base.table_name_prefix + expected + ActiveRecord::Base.table_name_suffix expect(sut.remove_prefix_and_suffix(tn)).to eq(expected) end -end + + [ + [true, 10, { timeout_seconds: 10 }], + [true, nil, {}], + [false, nil, {}] + ].each do |with_lock, timeout, expected_options| + context "when with_advisory_lock is #{with_lock} and timeout is #{timeout}" do + let(:options) { { with_advisory_lock: with_lock, advisory_lock_timeout_seconds: timeout } } + + it "uses advisory lock with timeout options: #{expected_options}" do + if with_lock + expect(model).to receive(:with_advisory_lock!).with(anything, expected_options).and_yield + else + expect(model).not_to receive(:with_advisory_lock!) + end + + expect { |b| sut.with_advisory_lock!(&b) }.to yield_control + end + end + end +end \ No newline at end of file