handle Timeout::Error in redis caching

Hook into the redis library at a pretty low level, to try and do
everything we can to avoid erroring if redis goes down. This applies to
both redis-as-cache and redis-as-data-store.

test plan: Set up redis and caching in your local instance. Point it to
both an existing box on a port not running redis, and a non-existent IP.
In both situations, you should not see caching errors or redis data
errors. After the first error, it shouldn't attempt to hit redis again for 5
minutes.

Change-Id: I101b2d3d2123151b244eb82ba78b176ed1f4d5ad
Reviewed-on: https://gerrit.instructure.com/8097
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>

app/models/page_view.rb

@@ -117,12 +117,13 @@ class PageView < ActiveRecord::Base
     when :log
       Rails.logger.info "PAGE VIEW: #{self.attributes.to_json}"
     when :cache
-      begin
-        json = self.attributes.as_json
-        json['is_update'] = true if self.is_update
-        Canvas.redis.rpush(PageView.cache_queue_name, json.to_json)
-      rescue Exception
-        # we're going to ignore the error for now, if redis is unavailable
+      json = self.attributes.as_json
+      json['is_update'] = true if self.is_update
+      if Canvas.redis.rpush(PageView.cache_queue_name, json.to_json)
+        true
+      else
+        # redis failed, push right to the db
+        self.save
       end
     when :db
       self.save

lib/canvas/redis.rb

@@ -25,45 +25,53 @@ module Canvas::Redis
     "lock:#{key}"
   end
 
+  def self.redis_failure?
+    return false unless @last_redis_failure
+    # i feel this dangling rescue is justifiable, given the try-to-be-failsafe nature of this code
+    return (Time.now - @last_redis_failure) < (Setting.get_cached('redis_failure_time', '300').to_i rescue 300)
+  end
+
+  def self.reset_redis_failure
+    @last_redis_failure = nil
+  end
+
+  def self.handle_redis_failure(failure_retval)
+    return failure_retval if redis_failure?
+    yield
+  rescue Errno::ECONNREFUSED, Timeout::Error, Errno::ECONNRESET, Errno::EPIPE, Errno::ECONNABORTED, Errno::EBADF, Errno::EINVAL
+    @last_redis_failure = Time.now
+    failure_retval
+  end
+
   # while we wait for this pull request
   # https://github.com/jodosha/redis-store/pull/83
   def self.patch
+    return if @redis_patched
+    Redis::Client.class_eval do
+      def process_with_conn_error(commands, *a, &b)
+        # try to return the type of value the command would expect, for some
+        # specific commands that we know can cause problems if we just return
+        # nil
+        #
+        # this isn't fool-proof, and in some situations it would probably
+        # actually be nicer to raise the exception and let the app handle it,
+        # but it's what we're going with for now
+        #
+        # for instance, Rails.cache.delete_matched will error out if the 'keys' command returns nil instead of []
+        last_command = commands.try(:last)
+        failure_val = case (last_command.respond_to?(:first) ? last_command.first : last_command).to_s
+          when 'keys'
+            []
+          else
+            nil
+        end
 
-    ::ActiveSupport::Cache::RedisStore.class_eval do
-      def write_with_econnrefused(key, value, options = nil)
-        write_without_econnrefused(key, value, options)
-      rescue Errno::ECONNREFUSED => e
-        false
+        Canvas::Redis.handle_redis_failure(failure_val) do
+          process_without_conn_error(commands, *a, &b)
+        end
       end
-      alias_method_chain :write, :econnrefused
-
-      def read_with_econnrefused(key, options = nil)
-        read_without_econnrefused(key, options)
-      rescue Errno::ECONNREFUSED => e
-        nil
-      end
-      alias_method_chain :read, :econnrefused
-
-      def delete_with_econnrefused(key, options = nil)
-        delete_without_econnrefused(key, options)
-      rescue Errno::ECONNREFUSED => e
-        false
-      end
-      alias_method_chain :delete, :econnrefused
-
-      def exist_with_econnrefused?(key, options = nil)
-        exist_without_econnrefused?(key, options = nil)
-      rescue Errno::ECONNREFUSED => e
-        false
-      end
-      alias_method_chain :exist?, :econnrefused
-
-      def delete_matched_with_econnrefused(matcher, options = nil)
-        delete_matched_without_econnrefused(matcher, options)
-      rescue Errno::ECONNREFUSED => e
-        false
-      end
-      alias_method_chain :delete_matched, :econnrefused
+      alias_method_chain :process, :conn_error
     end
+    @redis_patched = true
   end
 end

spec/lib/canvas/redis_spec.rb

@@ -45,5 +45,58 @@ describe "Canvas::Redis" do
       ttl.should <= 15
     end
   end
+
+  describe "redis failure" do
+    before do
+      Canvas::Redis.patch
+      Redis::Client.any_instance.expects(:ensure_connected).raises(Timeout::Error).once
+    end
+
+    after do
+      Canvas::Redis.reset_redis_failure
+    end
+
+    it "should not fail cache.read" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.read('blah').should == nil
+      end
+    end
+
+    it "should not call redis again after an error" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.read('blah').should == nil
+        # call again, the .once means that if it hits Redis::Client again it'll fail
+        Rails.cache.read('blah').should == nil
+      end
+    end
+
+    it "should not fail cache.write" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.write('blah', 'someval').should == nil
+      end
+    end
+
+    it "should not fail cache.delete" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.delete('blah').should == nil
+      end
+    end
+
+    it "should not fail cache.exist?" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.exist?('blah').should == nil
+      end
+    end
+
+    it "should not fail cache.delete_matched" do
+      enable_cache(ActiveSupport::Cache::RedisStore.new(['redis://localhost:1234'])) do
+        Rails.cache.delete_matched('blah').should == false
+      end
+    end
+
+    it "should not fail raw redis commands" do
+      Canvas.redis.setnx('my_key', 5).should == nil
+    end
+  end
 end
 end

spec/models/page_view_spec.rb

@@ -31,13 +31,24 @@ describe PageView do
   end
 
   if Canvas.redis_enabled?
-    it "should store into redis through to the db in cache mode" do
+    before do
       Setting.set('enable_page_views', 'cache')
+    end
+
+    it "should store into redis through to the db in cache mode" do
       @page_view.store.should be_true
       PageView.count.should == 0
       PageView.process_cache_queue
       PageView.count.should == 1
       PageView.first.attributes.except('created_at', 'updated_at').should == @page_view.attributes.except('created_at', 'updated_at')
     end
+
+    it "should store directly to the db if redis is down" do
+      Canvas::Redis.patch
+      Redis::Client.any_instance.expects(:ensure_connected).raises(Timeout::Error)
+      @page_view.store.should be_true
+      PageView.count.should == 1
+      PageView.first.attributes.except('created_at', 'updated_at').should == @page_view.attributes.except('created_at', 'updated_at')
+    end
   end
 end

spec/spec_helper.rb

@@ -115,7 +115,7 @@ Spec::Runner.configure do |config|
   end
   config.before :each do
     if Canvas.redis_enabled? && Canvas.redis_used
-      Canvas.redis.flushdb
+      Canvas.redis.flushdb rescue nil
     end
     Canvas.redis_used = false
   end
@@ -532,9 +532,8 @@ Spec::Runner.configure do |config|
     importer.warnings.should == []
   end
 
-  def enable_cache
+  def enable_cache(new_cache = ActiveSupport::Cache::MemoryStore.new)
     old_cache = RAILS_CACHE
-    new_cache = ActiveSupport::Cache::MemoryStore.new
     ActionController::Base.cache_store = new_cache
     silence_warnings { Object.const_set(:RAILS_CACHE, new_cache) }
     old_perform_caching = ActionController::Base.perform_caching