Skip to content
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

Avoid recalculating CounterCache.counter_cached_association_names #49917

Conversation

fatkodima
Copy link
Member

After merging #49866, a flaky test started to appear - https://buildkite.com/rails/rails/builds/101543#018b9610-f396-4e76-90e9-5bae14521f1e and https://buildkite.com/rails/rails/builds/101530#018b9485-eef8-49a9-8826-be82230fe7fe

from /rails/activesupport/lib/active_support/core_ext/module/redefine_method.rb:20:in `define_method'
from /rails/activesupport/lib/active_support/core_ext/module/redefine_method.rb:20:in `redefine_method'
from /rails/activesupport/lib/active_support/core_ext/module/redefine_method.rb:27:in `redefine_singleton_method'
from /rails/activerecord/lib/active_record/counter_cache.rb:10:in `counter_cached_association_names='
from /rails/activerecord/lib/active_record/counter_cache.rb:194:in `load_schema'
from /rails/activerecord/lib/active_record/model_schema.rb:423:in `columns_hash'
from /rails/activerecord/lib/active_record/locking/optimistic.rb:161:in `locking_enabled?'
from /rails/activerecord/lib/active_record/locking/optimistic.rb:60:in `locking_enabled?'
from /rails/activerecord/lib/active_record/locking/optimistic.rb:148:in `_query_constraints_hash'
from /rails/activerecord/lib/active_record/persistence.rb:944:in `update_columns'
from /rails/activerecord/lib/active_record/encryption/encryptable_record.rb:191:in `block in decrypt_attributes'
from /rails/activerecord/lib/active_record/encryption/contexts.rb:40:in `with_encryption_context'
from /rails/activerecord/lib/active_record/encryption/contexts.rb:50:in `without_encryption'
from /rails/activerecord/lib/active_record/encryption/encryptable_record.rb:191:in `decrypt_attributes'
from /rails/activerecord/lib/active_record/encryption/encryptable_record.rb:166:in `decrypt'
from /rails/activerecord/test/cases/encryption/concurrency_test.rb:22:in `block (2 levels) in thread_encrypting_and_decrypting'
from /rails/activerecord/test/cases/encryption/concurrency_test.rb:20:in `each'
from /rails/activerecord/test/cases/encryption/concurrency_test.rb:20:in `with_index'
from /rails/activerecord/test/cases/encryption/concurrency_test.rb:20:in `block in thread_encrypting_and_decrypting'

The failing line is the

which calls down the road

def locking_enabled?
lock_optimistically && columns_hash[locking_column]
end

which calls columns_hash which always calls load_schema which always recalculates and reassigns counter_cached_association_names https://github.com/rails/rails/pull/49866/files#diff-0441501ea93f2580c66bc58d845fcc2ad9dc5110cce249ce184c0b59483e253bR188-R194

Since the failing test from the referenced builds uses threads and counter_cached_association_names is defined as a class_attribute which is defined via

redefine_singleton_method(:#{name}) { value }
which calls redefined_method and which is defined as
def redefine_method(method, &block)
visibility = method_visibility(method)
silence_redefinition_of_method(method)
define_method(method, &block)
send(visibility, method)
end

Lines 19-20 should be executed atomically, but I suspect, that Thread1 executes line 19, Thread2 executes line 19, then Thread1 executes line 20 and when Thread2 tries to execute line 20, it gets the mentioned error.

cc @byroot @nvasilevski

@fatkodima fatkodima force-pushed the avoid-recalculating-counter_cached_association_names branch from 252b62e to e1cb3d0 Compare November 4, 2023 13:41
@byroot
Copy link
Member

byroot commented Nov 4, 2023

Good catch.

@byroot byroot merged commit 2b5851a into rails:main Nov 4, 2023
4 checks passed
@fatkodima fatkodima deleted the avoid-recalculating-counter_cached_association_names branch November 4, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants