Skip to content

Commit

Permalink
Don't forcefully clear temporarily disabled query caches
Browse files Browse the repository at this point in the history
Followup: rails#50938

The behavior changed to always clear the query cache as soon
as it's disabled, on the assumption that once queries have been
performed without it, all bets are off.

However that isn't quite true, we can apply the same clearing
logic than when it's enabled. If you perform read only queries
without the cache, there is no reason to clear it.
  • Loading branch information
byroot authored and jenshenny committed Feb 23, 2024
1 parent 39272a9 commit 0c0b9e7
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 9 deletions.
Expand Up @@ -29,7 +29,7 @@ def #{method_name}(...)
end

class Store # :nodoc:
attr_reader :enabled
attr_accessor :enabled
alias_method :enabled?, :enabled

def initialize(max_size)
Expand All @@ -38,11 +38,6 @@ def initialize(max_size)
@enabled = false
end

def enabled=(enabled)
clear if @enabled && !enabled
@enabled = enabled
end

def size
@map.size
end
Expand Down
13 changes: 11 additions & 2 deletions activerecord/lib/active_record/query_cache.rb
Expand Up @@ -8,7 +8,13 @@ module ClassMethods
# If it's not, it will execute the given block.
def cache(&block)
if connected? || !configurations.empty?
connection_pool.enable_query_cache(&block)
pool = connection_pool
was_enabled = pool.query_cache_enabled
begin
pool.enable_query_cache(&block)
ensure
pool.clear_query_cache unless was_enabled
end
else
yield
end
Expand All @@ -30,7 +36,10 @@ def self.run
end

def self.complete(pools)
pools.each(&:disable_query_cache!)
pools.each do |pool|
pool.disable_query_cache!
pool.clear_query_cache
end

ActiveRecord::Base.connection_handler.each_connection_pool do |pool|
pool.release_connection if pool.active_connection? && !pool.connection.transaction_open?
Expand Down
20 changes: 19 additions & 1 deletion activerecord/test/cases/query_cache_test.rb
Expand Up @@ -91,6 +91,24 @@ def test_writes_should_always_clear_cache
assert_cache :off
end

def test_reads_dont_clear_disabled_cache
assert_cache :off

mw = middleware { |env|
Post.first
query_cache = ActiveRecord::Base.connection.query_cache
assert_equal 1, query_cache.size, query_cache.inspect
Post.connection.uncached do
Post.count # shouldn't clear the cache
end
query_cache = ActiveRecord::Base.connection.query_cache
assert_equal 1, query_cache.size, query_cache.inspect
}
mw.call({})

assert_cache :off
end

def test_exceptional_middleware_clears_and_disables_cache_on_error
assert_cache :off

Expand Down Expand Up @@ -808,7 +826,7 @@ def test_cache_gets_cleared_after_migration
end

def test_find
assert_called(Task.connection.query_cache, :clear, times: 2) do
assert_called(Task.connection.query_cache, :clear, times: 1) do
assert_not Task.connection.query_cache_enabled
Task.cache do
assert Task.connection.query_cache_enabled
Expand Down

0 comments on commit 0c0b9e7

Please sign in to comment.