From afb5ec811df2c403644db274e908d44c9e2f34ce Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 21 Jun 2019 15:08:57 +0100 Subject: [PATCH] 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. --- app/controllers/application_controller.rb | 2 +- lib/discourse.rb | 33 ++++++++++++++++----- lib/discourse_redis.rb | 2 +- spec/components/discourse_spec.rb | 26 +++++++++++----- spec/requests/forums_controller_spec.rb | 2 +- spec/requests/topics_controller_spec.rb | 2 +- spec/services/random_topic_selector_spec.rb | 2 +- 7 files changed, 49 insertions(+), 20 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d51265bde4c..1798ba5e53b 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 2b53a91eb37..f1adb928c27 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 85aa7077b51..d62113d7519 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 391691ff9bf..9b3dc50b4ca 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_readonly! - expect(Discourse.last_read_only['default']).to eq(time) + 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_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 c288909dd60..865e7b94845 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 d454dd9f2eb..6aac3a99ace 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 c601523aebd..4f4fe5a94bb 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)