From d27d6c84fd1ca3e38eb40fe3a08e7a9219941aa5 Mon Sep 17 00:00:00 2001
From: Robin Clowers <robin.clowers@gmail.com>
Date: Wed, 2 Dec 2015 21:10:08 -0800
Subject: Fix cache fetch miss notification order

Fixes https://github.com/rails/rails/issues/22477.

When I improved the caching instrumentation in
edd33c08d98723ae9bb89cf7f019277117ed6414, I inadvertently changed the
order of AS notifications when there is a cache miss.
---
 activesupport/lib/active_support/cache.rb | 16 ++++++++--------
 activesupport/test/caching_test.rb        | 28 ++++++++++++++++------------
 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
-- 
cgit v1.2.3