Merge pull request #22479 from RobinClowers/fix-cache-notification-order

Fix cache fetch miss notification order

2 files changed, 24 insertions, 20 deletions

diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index 5011014e96..8272d3395c 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -278,18 +278,18 @@ module ActiveSupport
           options = merged_options(options)
           key = normalize_key(name, options)
 
+          entry = nil
           instrument(:read, name, options) do |payload|
             cached_entry = read_entry(key, options) unless options[:force]
-            payload[:super_operation] = :fetch if payload
             entry = handle_expired_entry(cached_entry, key, options)
+            payload[:super_operation] = :fetch if payload
+            payload[:hit] = !!entry if payload
+          end
 
-            if entry
-              payload[:hit] = true if payload
-              get_entry_value(entry, name, options)
-            else
-              payload[:hit] = false if payload
-              save_block_result_to_cache(name, options) { |_name| yield _name }
-            end
+          if entry
+            get_entry_value(entry, name, options)
+          else
+            save_block_result_to_cache(name, options) { |_name| yield _name }
           end
         else
           read(name, options)
diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
index 7bef73136c..465b943173 100644
--- a/activesupport/test/caching_test.rb
+++ b/activesupport/test/caching_test.rb
@@ -493,28 +493,32 @@ module CacheStoreBehavior
 
   def test_cache_hit_instrumentation
     key = "test_key"
-    subscribe_executed = false
-    ActiveSupport::Notifications.subscribe "cache_read.active_support" do |name, start, finish, id, payload|
-      subscribe_executed = true
-      assert_equal :fetch, payload[:super_operation]
-      assert payload[:hit]
+    @events = []
+    ActiveSupport::Notifications.subscribe "cache_read.active_support" do |*args|
+      @events << ActiveSupport::Notifications::Event.new(*args)
     end
     assert @cache.write(key, "1", :raw => true)
     assert @cache.fetch(key) {}
-    assert subscribe_executed
+    assert_equal 1, @events.length
+    assert_equal 'cache_read.active_support', @events[0].name
+    assert_equal :fetch, @events[0].payload[:super_operation]
+    assert @events[0].payload[:hit]
   ensure
     ActiveSupport::Notifications.unsubscribe "cache_read.active_support"
   end
 
   def test_cache_miss_instrumentation
-    subscribe_executed = false
-    ActiveSupport::Notifications.subscribe "cache_read.active_support" do |name, start, finish, id, payload|
-      subscribe_executed = true
-      assert_equal :fetch, payload[:super_operation]
-      assert_not payload[:hit]
+    @events = []
+    ActiveSupport::Notifications.subscribe /^cache_(.*)\.active_support$/ do |*args|
+      @events << ActiveSupport::Notifications::Event.new(*args)
     end
     assert_not @cache.fetch("bad_key") {}
-    assert subscribe_executed
+    assert_equal 3, @events.length
+    assert_equal 'cache_read.active_support', @events[0].name
+    assert_equal 'cache_generate.active_support', @events[1].name
+    assert_equal 'cache_write.active_support', @events[2].name
+    assert_equal :fetch, @events[0].payload[:super_operation]
+    assert_not @events[0].payload[:hit]
   ensure
     ActiveSupport::Notifications.unsubscribe "cache_read.active_support"
   end