From b92194496387eb7b0d9e8323c05949c20a7b66f1 Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:23:32 +0100 Subject: [PATCH 1/7] Make recursive association hydration thread-safe (#568) --- .gitignore | 17 ++++++++++ CHANGELOG.md | 3 ++ .../cached/recursive/association.rb | 31 ++++++++++++++++--- test/cached/recursive/has_many_test.rb | 27 ++++++++++++++++ 4 files changed, 74 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 77900dd1..43a15e3a 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,20 @@ tmp .rubocop-http* .byebug_history gemfiles/*.lock + +# Local bundle path and vendored gems +vendor/ +vendor/bundle/ + +# Editor/OS files +.idea/ +.vscode/ +.DS_Store + +# Ruby version managers and env files +.ruby-version +.tool-versions +.env + +# Logs +log/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b3a3ec5..f2e824d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- Fix rare race condition causing `NameError` (missing `@dehydrated_*` ivar) when hydrating recursively + embedded associations by synchronizing hydration to be thread-safe. + ## 1.6.3 - Split the `with_deferred_parent_expiration` and `with_deferred_parent_expiration`. (#578) diff --git a/lib/identity_cache/cached/recursive/association.rb b/lib/identity_cache/cached/recursive/association.rb index 6b573b2e..70f5f5be 100644 --- a/lib/identity_cache/cached/recursive/association.rb +++ b/lib/identity_cache/cached/recursive/association.rb @@ -7,6 +7,7 @@ class Association < Cached::Association # :nodoc: def initialize(name, reflection:) super @dehydrated_variable_name = :"@dehydrated_#{name}" + @hydration_mutex = Mutex.new end attr_reader :dehydrated_variable_name @@ -29,10 +30,7 @@ def read(record) if record.instance_variable_defined?(records_variable_name) record.instance_variable_get(records_variable_name) elsif record.instance_variable_defined?(dehydrated_variable_name) - dehydrated_target = record.instance_variable_get(dehydrated_variable_name) - association_target = hydrate_association_target(assoc.klass, dehydrated_target) - record.remove_instance_variable(dehydrated_variable_name) - set_with_inverse(record, association_target) + hydrate_safely(record, assoc) else assoc.load_target end @@ -76,6 +74,31 @@ def embedded_recursively? private + # Prevents races during hydration, where multiple threads could + # simultaneously try to remove the same dehydrated ivar. + # + # Race condition scenario: + # - Thread A and Thread B both call read() on the same record. + # - Both check record.instance_variable_defined?(dehydrated_variable_name) => true + # - Both proceed to hydrate and remove the ivar. + # - Thread A removes it first, Thread B gets NameError when trying to remove. + # + # The mutex ensures only one thread hydrates at a time, preventing the error. + def hydrate_safely(record, assoc) + @hydration_mutex.synchronize do + if record.instance_variable_defined?(records_variable_name) + return record.instance_variable_get(records_variable_name) + end + + dehydrated_target = record.instance_variable_get(dehydrated_variable_name) + association_target = hydrate_association_target(assoc.klass, dehydrated_target) + if record.instance_variable_defined?(dehydrated_variable_name) + record.remove_instance_variable(dehydrated_variable_name) + end + set_with_inverse(record, association_target) + end + end + def set_inverse(record, association_target) return if association_target.nil? diff --git a/test/cached/recursive/has_many_test.rb b/test/cached/recursive/has_many_test.rb index 349f5868..3e3ddb3d 100644 --- a/test/cached/recursive/has_many_test.rb +++ b/test/cached/recursive/has_many_test.rb @@ -45,6 +45,33 @@ def test_embedded_recursively def test_embedded_by_reference refute_predicate(has_many, :embedded_by_reference?) end + + def test_thread_safety_hydration + # Simulate race condition by setting up a dehydrated record + record = AssociatedRecord.new + dehydrated_data = [{ "id" => 1, "name" => "Test" }] # Mock dehydrated data + record.instance_variable_set(has_many.dehydrated_variable_name, dehydrated_data) + + # Simulate multiple threads trying to hydrate concurrently + threads = [] + errors = [] + + 10.times do + threads << Thread.new do + begin + # Call the cached accessor which triggers hydration + record.fetch_deeply_associated_records + rescue => e + errors << e + end + end + end + + threads.each(&:join) + + # Assert no NameError occurred (which would indicate the race condition) + assert_empty(errors, "Race condition detected: #{errors.map(&:message).join(', ')}") + end end end end From 5038cb1f356bf4e06e2a37c3c03db9b3fc202627 Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:26:16 +0100 Subject: [PATCH 2/7] Remove outdated comments from recursive association hydration method --- lib/identity_cache/cached/recursive/association.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/lib/identity_cache/cached/recursive/association.rb b/lib/identity_cache/cached/recursive/association.rb index 70f5f5be..5bee1ff0 100644 --- a/lib/identity_cache/cached/recursive/association.rb +++ b/lib/identity_cache/cached/recursive/association.rb @@ -74,16 +74,6 @@ def embedded_recursively? private - # Prevents races during hydration, where multiple threads could - # simultaneously try to remove the same dehydrated ivar. - # - # Race condition scenario: - # - Thread A and Thread B both call read() on the same record. - # - Both check record.instance_variable_defined?(dehydrated_variable_name) => true - # - Both proceed to hydrate and remove the ivar. - # - Thread A removes it first, Thread B gets NameError when trying to remove. - # - # The mutex ensures only one thread hydrates at a time, preventing the error. def hydrate_safely(record, assoc) @hydration_mutex.synchronize do if record.instance_variable_defined?(records_variable_name) From a01ccd20273255a40b60a92f794f51e6a2042e3a Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:29:20 +0100 Subject: [PATCH 3/7] Update .gitignore to remove unnecessary entries and retain only gemfiles lock file --- .gitignore | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index 43a15e3a..a22c2b2a 100644 --- a/.gitignore +++ b/.gitignore @@ -16,21 +16,4 @@ test/version_tmp tmp .rubocop-http* .byebug_history -gemfiles/*.lock - -# Local bundle path and vendored gems -vendor/ -vendor/bundle/ - -# Editor/OS files -.idea/ -.vscode/ -.DS_Store - -# Ruby version managers and env files -.ruby-version -.tool-versions -.env - -# Logs -log/ +gemfiles/*.lock \ No newline at end of file From 90feae8955a587c549fd00ef63feaf166ecd8db1 Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:30:45 +0100 Subject: [PATCH 4/7] Update .gitignore to add newline at the end of the file for consistency --- .gitignore | 2 +- vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 160000 vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 diff --git a/.gitignore b/.gitignore index a22c2b2a..77900dd1 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,4 @@ test/version_tmp tmp .rubocop-http* .byebug_history -gemfiles/*.lock \ No newline at end of file +gemfiles/*.lock diff --git a/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 b/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 new file mode 160000 index 00000000..30f5735d --- /dev/null +++ b/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 @@ -0,0 +1 @@ +Subproject commit 30f5735d5db2dfd50dd885ceeb51672320b356d3 From 529e6084c4dd35cd6f167dc46e4613108b5dc519 Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:32:14 +0100 Subject: [PATCH 5/7] Update .gitignore to ensure consistent formatting by adding a newline at the end of the file --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 77900dd1..a22c2b2a 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,4 @@ test/version_tmp tmp .rubocop-http* .byebug_history -gemfiles/*.lock +gemfiles/*.lock \ No newline at end of file From 9659e90e3d4b8dbafc767212f56fb2ba15e9e56d Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:32:59 +0100 Subject: [PATCH 6/7] Remove memcached gem subproject as it is no longer needed --- vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 | 1 - 1 file changed, 1 deletion(-) delete mode 160000 vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 diff --git a/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 b/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 deleted file mode 160000 index 30f5735d..00000000 --- a/vendor/bundle/ruby/3.0.0/bundler/gems/memcached-30f5735d5db2 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 30f5735d5db2dfd50dd885ceeb51672320b356d3 From 85ca3a6fcaf4f058b1404cadce3600347f04f662 Mon Sep 17 00:00:00 2001 From: Merouane AMQOR Date: Thu, 18 Sep 2025 15:33:22 +0100 Subject: [PATCH 7/7] Update .gitignore to add missing newline at the end of the file for consistency --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index a22c2b2a..77900dd1 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,4 @@ test/version_tmp tmp .rubocop-http* .byebug_history -gemfiles/*.lock \ No newline at end of file +gemfiles/*.lock