Skip to content

Raise error when advisory lock cannot be acquired #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/closure_tree/finders.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions lib/closure_tree/has_closure_tree.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def has_closure_tree(options = {})
:numeric_order,
:touch,
:with_advisory_lock,
:advisory_lock_timeout_seconds,
:order_belong_to
)

Expand Down
8 changes: 4 additions & 4 deletions lib/closure_tree/hierarchy_maintenance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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! }
Expand All @@ -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?
Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/closure_tree/numeric_deterministic_ordering.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions lib/closure_tree/support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions lib/closure_tree/support_attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/closure_tree/parallel_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 26 additions & 2 deletions spec/closure_tree/support_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Loading