From db5bbb3c0474aeaa775b6f2a44f2d3435a30a981 Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Fri, 7 Jun 2024 15:54:48 +0300 Subject: [PATCH 1/8] Update platforms following rebase --- Gemfile.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile.lock b/Gemfile.lock index a082b7d2..c1c38201 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -86,6 +86,7 @@ GEM PLATFORMS arm64-darwin-22 + arm64-darwin-23 x86_64-linux DEPENDENCIES From b3a64e2880734eecc36a9279ccb50a20f88a6e4d Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Fri, 7 Jun 2024 16:00:22 +0300 Subject: [PATCH 2/8] Introduce `.with_deferred_parent_expiration` --- lib/identity_cache.rb | 11 ++++ lib/identity_cache/parent_model_expiration.rb | 4 ++ test/index_cache_test.rb | 64 +++++++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index 73d6b423..7a3ce7fd 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -191,6 +191,17 @@ def fetch_multi(*keys) result end + def with_deferred_parent_expiration + raise NestedDeferredParentBlockError if Thread.current[:deferred_parent_expiration] + + Thread.current[:deferred_parent_expiration] = true + Thread.current[:parent_records_for_cache_expiry] = Set.new + + yield + + Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index) + end + def with_fetch_read_only_records(value = true) old_value = Thread.current[:identity_cache_fetch_read_only_records] Thread.current[:identity_cache_fetch_read_only_records] = value diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index 1993a21c..440c7f4b 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -47,6 +47,10 @@ def expire_parent_caches add_parents_to_cache_expiry_set(parents_to_expire) parents_to_expire.select! { |parent| parent.class.primary_cache_index_enabled } parents_to_expire.reduce(true) do |all_expired, parent| + if Thread.current[:deferred_parent_expiration] + Thread.current[:parent_records_for_cache_expiry] << parent + next parent + end parent.expire_primary_index && all_expired end end diff --git a/test/index_cache_test.rb b/test/index_cache_test.rb index f80bd6cd..e4dcbc7b 100644 --- a/test/index_cache_test.rb +++ b/test/index_cache_test.rb @@ -166,6 +166,70 @@ def test_unique_cache_index_with_non_id_primary_key assert_equal(123, KeyedRecord.fetch_by_value("a").id) end + def test_with_deferred_parent_expiration_expires_parent_index_once + Item.send(:cache_has_many, :associated_records, embed: true) + + @parent = Item.create!(title: "bob") + @records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }]) + + @memcached_spy = Spy.on(backend, :write).and_call_through + + expected_item_expiration_count = Array(@parent).count + expected_associated_record_expiration_count = @records.count + + IdentityCache.with_deferred_parent_expiration do + @parent.transaction do + @parent.associated_records.destroy_all + end + assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count) + end + + expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first) + item_expiration_count = expired_cache_keys.count { _1.include?("Item") } + associated_record_expiration_count = expired_cache_keys.count { _1.include?("AssociatedRecord") } + + assert_operator(@memcached_spy.calls.count, :>, 0) + assert_equal(expected_item_expiration_count, item_expiration_count) + assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) + end + + def test_deep_association_with_deferred_parent_expiration_expires_parent_once + AssociatedRecord.send(:has_many, :deeply_associated_records, dependent: :destroy) + Item.send(:cache_has_many, :associated_records, embed: true) + + @parent = Item.create!(title: "bob") + @records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }]) + @records.each do + _1.deeply_associated_records.create!([ + { name: "a", item: @parent }, + { name: "b", item: @parent }, + { name: "c", item: @parent }, + ]) + end + + @memcached_spy = Spy.on(backend, :write).and_call_through + + expected_item_expiration_count = Array(@parent).count + expected_associated_record_expiration_count = @records.count + expected_deeply_associated_record_expiration_count = @records.flat_map(&:deeply_associated_records).count + + IdentityCache.with_deferred_parent_expiration do + @parent.transaction do + @parent.associated_records.destroy_all + end + end + + expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first) + item_expiration_count = expired_cache_keys.count { _1.include?("Item") } + associated_record_expiration_count = expired_cache_keys.count { _1.include?(":AssociatedRecord:") } + deeply_associated_record_expiration_count = expired_cache_keys.count { _1.include?("DeeplyAssociatedRecord") } + + assert_operator(@memcached_spy.calls.count, :>, 0) + assert_equal(expected_item_expiration_count, item_expiration_count) + assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) + assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_expiration_count) + end + private def cache_key(unique: false) From 702abe0a943b383cc8cef6c86202f884a13070c2 Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Mon, 10 Jun 2024 16:23:02 +0300 Subject: [PATCH 3/8] Add mechanism to prevent nested blocks This could have caused the cache to have been overwritten --- lib/identity_cache.rb | 3 +++ test/index_cache_test.rb | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index 7a3ce7fd..91d43bc8 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -60,6 +60,8 @@ class AssociationError < StandardError; end class InverseAssociationError < StandardError; end + class NestedDeferredParentBlockError < StandardError; end + class UnsupportedScopeError < StandardError; end class UnsupportedAssociationError < StandardError; end @@ -199,6 +201,7 @@ def with_deferred_parent_expiration yield + Thread.current[:deferred_parent_expiration] = nil Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index) end diff --git a/test/index_cache_test.rb b/test/index_cache_test.rb index e4dcbc7b..97eabb3e 100644 --- a/test/index_cache_test.rb +++ b/test/index_cache_test.rb @@ -191,6 +191,32 @@ def test_with_deferred_parent_expiration_expires_parent_index_once assert_operator(@memcached_spy.calls.count, :>, 0) assert_equal(expected_item_expiration_count, item_expiration_count) assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) + ensure + Thread.current[:deferred_parent_expiration] = nil + end + + def test_double_nested_deferred_parent_expiration + Item.send(:cache_has_many, :associated_records, embed: true) + + @parent = Item.create!(title: "bob") + @records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }]) + + @memcached_spy = Spy.on(backend, :write).and_call_through + + assert_raises(IdentityCache::NestedDeferredParentBlockError) do + IdentityCache.with_deferred_parent_expiration do + IdentityCache.with_deferred_parent_expiration do + @parent.transaction do + @parent.associated_records.destroy_all + end + assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count) + end + end + end + + assert_equal(0, @memcached_spy.calls.count) + ensure + Thread.current[:deferred_parent_expiration] = nil end def test_deep_association_with_deferred_parent_expiration_expires_parent_once @@ -228,6 +254,8 @@ def test_deep_association_with_deferred_parent_expiration_expires_parent_once assert_equal(expected_item_expiration_count, item_expiration_count) assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_expiration_count) + ensure + Thread.current[:deferred_parent_expiration] = nil end private From 168faa1a71779862ec5f79a7fea4b2cc9c83cafa Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Tue, 11 Jun 2024 14:15:03 +0300 Subject: [PATCH 4/8] Ensure the thread variables are reset when exiting the block --- lib/identity_cache.rb | 7 ++++++- test/index_cache_test.rb | 15 ++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index 91d43bc8..adc7b2da 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -199,10 +199,15 @@ def with_deferred_parent_expiration Thread.current[:deferred_parent_expiration] = true Thread.current[:parent_records_for_cache_expiry] = Set.new - yield + result = yield Thread.current[:deferred_parent_expiration] = nil Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index) + + result + ensure + Thread.current[:deferred_parent_expiration] = nil + Thread.current[:parent_records_for_cache_expiry].clear end def with_fetch_read_only_records(value = true) diff --git a/test/index_cache_test.rb b/test/index_cache_test.rb index 97eabb3e..bb3a48ea 100644 --- a/test/index_cache_test.rb +++ b/test/index_cache_test.rb @@ -177,11 +177,14 @@ def test_with_deferred_parent_expiration_expires_parent_index_once expected_item_expiration_count = Array(@parent).count expected_associated_record_expiration_count = @records.count - IdentityCache.with_deferred_parent_expiration do + expected_return_value = "Some text that we expect to see returned from the block" + + result = IdentityCache.with_deferred_parent_expiration do @parent.transaction do @parent.associated_records.destroy_all end assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count) + expected_return_value end expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first) @@ -191,11 +194,10 @@ def test_with_deferred_parent_expiration_expires_parent_index_once assert_operator(@memcached_spy.calls.count, :>, 0) assert_equal(expected_item_expiration_count, item_expiration_count) assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) - ensure - Thread.current[:deferred_parent_expiration] = nil + assert_equal(expected_return_value, result) end - def test_double_nested_deferred_parent_expiration + def test_double_nested_deferred_parent_expiration_will_raise_error Item.send(:cache_has_many, :associated_records, embed: true) @parent = Item.create!(title: "bob") @@ -209,14 +211,11 @@ def test_double_nested_deferred_parent_expiration @parent.transaction do @parent.associated_records.destroy_all end - assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count) end end end assert_equal(0, @memcached_spy.calls.count) - ensure - Thread.current[:deferred_parent_expiration] = nil end def test_deep_association_with_deferred_parent_expiration_expires_parent_once @@ -254,8 +253,6 @@ def test_deep_association_with_deferred_parent_expiration_expires_parent_once assert_equal(expected_item_expiration_count, item_expiration_count) assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_expiration_count) - ensure - Thread.current[:deferred_parent_expiration] = nil end private From fddbf83ab19919d600476c6fda66008d64b81f0f Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Wed, 12 Jun 2024 16:24:10 +0300 Subject: [PATCH 5/8] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8319c031..df11947d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Introduce `.with_deferred_parent_expiration`, which takes a block and avoids duplicate parent cache expiry. (#569) + ## 1.5.6 - Minor performance improvements on association read From 1aba576395d1f5286b2bdbd44bb1efbd31ebe2a3 Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Fri, 14 Jun 2024 15:10:26 +0300 Subject: [PATCH 6/8] Namespace `Thread.current` keys --- lib/identity_cache.rb | 14 +++++++------- lib/identity_cache/parent_model_expiration.rb | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index adc7b2da..c97b46e6 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -194,20 +194,20 @@ def fetch_multi(*keys) end def with_deferred_parent_expiration - raise NestedDeferredParentBlockError if Thread.current[:deferred_parent_expiration] + raise NestedDeferredParentBlockError if Thread.current[:idc_deferred_parent_expiration] - Thread.current[:deferred_parent_expiration] = true - Thread.current[:parent_records_for_cache_expiry] = Set.new + Thread.current[:idc_deferred_parent_expiration] = true + Thread.current[:idc_parent_records_for_cache_expiry] = Set.new result = yield - Thread.current[:deferred_parent_expiration] = nil - Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index) + Thread.current[:idc_deferred_parent_expiration] = nil + Thread.current[:idc_parent_records_for_cache_expiry].each(&:expire_primary_index) result ensure - Thread.current[:deferred_parent_expiration] = nil - Thread.current[:parent_records_for_cache_expiry].clear + Thread.current[:idc_deferred_parent_expiration] = nil + Thread.current[:idc_parent_records_for_cache_expiry].clear end def with_fetch_read_only_records(value = true) diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index 440c7f4b..499e6d0b 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -47,8 +47,8 @@ def expire_parent_caches add_parents_to_cache_expiry_set(parents_to_expire) parents_to_expire.select! { |parent| parent.class.primary_cache_index_enabled } parents_to_expire.reduce(true) do |all_expired, parent| - if Thread.current[:deferred_parent_expiration] - Thread.current[:parent_records_for_cache_expiry] << parent + if Thread.current[:idc_deferred_parent_expiration] + Thread.current[:idc_parent_records_for_cache_expiry] << parent next parent end parent.expire_primary_index && all_expired From 7f701baa07d87c1bd5bd051f3056bed9878b105e Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Tue, 2 Jul 2024 20:31:39 +0300 Subject: [PATCH 7/8] Add rdoc documentation --- lib/identity_cache.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index c97b46e6..15fd653e 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -193,6 +193,27 @@ def fetch_multi(*keys) result end + # Executes a block with deferred parent expiration, ensuring that the parent + # records' cache expiration is deferred until the block completes. When the block + # completes, it triggers expiration of the primary index for the parent records. + # Raises a NestedDeferredParentBlockError if a deferred parent expiration block + # is already active on the current thread. + # + # == Parameters: + # No parameters. + # + # == Raises: + # NestedDeferredParentBlockError if a deferred parent expiration block is already active. + # + # == Yield: + # Runs the provided block with deferred parent expiration. + # + # == Returns: + # The result of executing the provided block. + # + # == Ensures: + # Cleans up thread-local variables related to deferred parent expiration regardless + # of whether the block raises an exception. def with_deferred_parent_expiration raise NestedDeferredParentBlockError if Thread.current[:idc_deferred_parent_expiration] From 1d3e37ee2a0490c7744ce934cb65979581f952f2 Mon Sep 17 00:00:00 2001 From: Efstathios Stivaros Date: Wed, 3 Jul 2024 16:20:17 +0300 Subject: [PATCH 8/8] Add version number --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index df11947d..24152319 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +## 1.6.0 + - Introduce `.with_deferred_parent_expiration`, which takes a block and avoids duplicate parent cache expiry. (#569) ## 1.5.6