FIX: Don't use DistributedCache to store redis readonly state

FIX: Don’t use DistributedCache to store redis readonly state

This can cause unbound CPU usage in some cases, and excessive logging in other cases. This commit moves redis readonly information into the local process, but maintains the DistributedCache for postgres readonly state.

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index d51265b..1798ba5 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -139,7 +139,7 @@ class ApplicationController < ActionController::Base
   end
 
   rescue_from PG::ReadOnlySqlTransaction do |e|
-    Discourse.received_readonly!
+    Discourse.received_postgres_readonly!
     Rails.logger.error("#{e.class} #{e.message}: #{e.backtrace.join("\n")}")
     raise Discourse::ReadOnly
   end
diff --git a/lib/discourse.rb b/lib/discourse.rb
index 2b53a91..f1adb92 100644
--- a/lib/discourse.rb
+++ b/lib/discourse.rb
@@ -377,21 +377,34 @@ module Discourse
     $redis.get(PG_READONLY_MODE_KEY).present?
   end
 
-  def self.last_read_only
-    @last_read_only ||= DistributedCache.new('last_read_only', namespace: false)
+  # Shared between processes
+  def self.postgres_last_read_only
+    @postgres_last_read_only ||= DistributedCache.new('postgres_last_read_only', namespace: false)
+  end
+
+  # Per-process
+  def self.redis_last_read_only
+    @redis_last_read_only ||= {}
   end
 
   def self.recently_readonly?
-    read_only = last_read_only[$redis.namespace]
-    read_only.present? && read_only > 15.seconds.ago
+    postgres_read_only = postgres_last_read_only[$redis.namespace]
+    redis_read_only = redis_last_read_only[$redis.namespace]
+
+    (redis_read_only.present? && redis_read_only > 15.seconds.ago) ||
+      (postgres_read_only.present? && postgres_read_only > 15.seconds.ago)
   end
 
-  def self.received_readonly!
-    last_read_only[$redis.namespace] = Time.zone.now
+  def self.received_postgres_readonly!
+    postgres_last_read_only[$redis.namespace] = Time.zone.now
+  end
+
+  def self.received_redis_readonly!
+    redis_last_read_only[$redis.namespace] = Time.zone.now
   end
 
   def self.clear_readonly!
-    last_read_only[$redis.namespace] = nil
+    postgres_last_read_only[$redis.namespace] = redis_last_read_only[$redis.namespace] = nil
     Site.clear_anon_cache!
     true
   end
@@ -667,7 +680,11 @@ module Discourse
 
     if !$redis.without_namespace.get(redis_key)
       Rails.logger.warn(warning)
-      $redis.without_namespace.setex(redis_key, 3600, "x")
+      begin
+        $redis.without_namespace.setex(redis_key, 3600, "x")
+      rescue Redis::CommandError => e
+        raise unless e.message =~ /READONLY/
+      end
     end
     warning
   end
diff --git a/lib/discourse_redis.rb b/lib/discourse_redis.rb
index 85aa707..d62113d 100644
--- a/lib/discourse_redis.rb
+++ b/lib/discourse_redis.rb
@@ -176,7 +176,7 @@ class DiscourseRedis
       end
 
       fallback_handler.verify_master if !fallback_handler.master
-      Discourse.received_readonly!
+      Discourse.received_redis_readonly!
       nil
     else
       raise ex
diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb
index 391691f..9b3dc50 100644
--- a/spec/components/discourse_spec.rb
+++ b/spec/components/discourse_spec.rb
@@ -219,8 +219,13 @@ describe Discourse do
         expect(Discourse.readonly_mode?).to eq(true)
       end
 
-      it "returns true when Discourse is recently read only" do
-        Discourse.received_readonly!
+      it "returns true when postgres is recently read only" do
+        Discourse.received_postgres_readonly!
+        expect(Discourse.readonly_mode?).to eq(true)
+      end
+
+      it "returns true when redis is recently read only" do
+        Discourse.received_redis_readonly!
         expect(Discourse.readonly_mode?).to eq(true)
       end
 
@@ -234,21 +239,28 @@ describe Discourse do
       end
     end
 
-    describe ".received_readonly!" do
+    describe ".received_postgres_readonly!" do
+      it "sets the right time" do
+        time = Discourse.received_postgres_readonly!
+        expect(Discourse.postgres_last_read_only['default']).to eq(time)
+      end
+    end
+
+    describe ".received_redis_readonly!" do
       it "sets the right time" do
-        time = Discourse.received_readonly!
-        expect(Discourse.last_read_only['default']).to eq(time)
+        time = Discourse.received_redis_readonly!
+        expect(Discourse.redis_last_read_only['default']).to eq(time)
       end
     end
 
     describe ".clear_readonly!" do
       it "publishes the right message" do
-        Discourse.received_readonly!
+        Discourse.received_postgres_readonly!
         messages = []
 
         expect do
           messages = MessageBus.track_publish { Discourse.clear_readonly! }
-        end.to change { Discourse.last_read_only['default'] }.to(nil)
+        end.to change { Discourse.postgres_last_read_only['default'] }.to(nil)
 
         expect(messages.any? { |m| m.channel == Site::SITE_JSON_CHANNEL })
           .to eq(true)
diff --git a/spec/requests/forums_controller_spec.rb b/spec/requests/forums_controller_spec.rb
index c288909..865e7b9 100644
--- a/spec/requests/forums_controller_spec.rb
+++ b/spec/requests/forums_controller_spec.rb
@@ -12,7 +12,7 @@ RSpec.describe ForumsController do
     end
 
     it "returns a readonly header if the site is read only" do
-      Discourse.received_readonly!
+      Discourse.received_postgres_readonly!
       get "/srv/status"
       expect(response.status).to eq(200)
       expect(response.headers['Discourse-Readonly']).to eq('true')
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index d454dd9..6aac3a9 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -1762,7 +1762,7 @@ RSpec.describe TopicsController do
       end
 
       it "returns a readonly header if the site is read only" do
-        Discourse.received_readonly!
+        Discourse.received_postgres_readonly!
         get "/t/#{topic.id}.json"
         expect(response.status).to eq(200)
         expect(response.headers['Discourse-Readonly']).to eq('true')
diff --git a/spec/services/random_topic_selector_spec.rb b/spec/services/random_topic_selector_spec.rb
index c601523..4f4fe5a 100644
--- a/spec/services/random_topic_selector_spec.rb
+++ b/spec/services/random_topic_selector_spec.rb
@@ -16,7 +16,7 @@ describe RandomTopicSelector do
     expect(RandomTopicSelector.next(0)).to eq([])
     expect(RandomTopicSelector.next(2)).to eq([0, 1])
 
-    $redis.expects(:multi).returns(Discourse.received_readonly!)
+    $redis.expects(:multi).returns(Discourse.received_redis_readonly!)
     expect(RandomTopicSelector.next(2)).to eq([2, 3])
     $redis.unstub(:multi)

GitHub sha: afb5ec81

1 Like