Merge pull request #498 from alphagov/catch-dalli-errors

Catch errors from the Dalli client

Author: h-lame

app/lib/active_support/cache/atomic_dalli_store.rb

@@ -34,13 +34,11 @@ def read(name, options = nil)
       def write(name, value, options = nil)
         expiry = (options && options[:expires_in]) || 0
         options[:expires_in] = expiry + 20 unless expiry.zero?
-        ttl_set(ttl_key(name, options), expiry)
-        super
+        ttl_set(ttl_key(name, options), expiry) && super
       end
 
       def delete(name, options = nil)
-        ttl_delete(ttl_key(name, options))
-        super
+        ttl_delete(ttl_key(name, options)) && super
       end
 
       private
@@ -56,18 +54,34 @@ def ttl_key(name, options)
 
       def ttl_get(key)
         with { |c| c.get(key, raw: true) }
+      rescue Dalli::DalliError => e
+        logger.error("DalliError: #{e.message}") if logger
+        raise if raise_errors?
+        nil
       end
 
       def ttl_add(key)
         with { |c| c.add(key, "", 10, raw: true) }
+      rescue Dalli::DalliError => e
+        logger.error("DalliError: #{e.message}") if logger
+        raise if raise_errors?
+        false
       end
 
       def ttl_set(key, expiry)
         with { |c| c.set(key, "", expiry, raw: true) }
+      rescue Dalli::DalliError => e
+        logger.error("DalliError: #{e.message}") if logger
+        raise if raise_errors?
+        false
       end
 
       def ttl_delete(key)
         with { |c| c.delete(key) }
+      rescue Dalli::DalliError => e
+        logger.error("DalliError: #{e.message}") if logger
+        raise if raise_errors?
+        false
       end
     end
   end

spec/lib/atomic_dalli_store_spec.rb

@@ -2,17 +2,28 @@
 
 RSpec.describe ActiveSupport::Cache::AtomicDalliStore do
   let(:options) do
-    { namespace: "epets_test", expires_in: 5.minutes }
+    { namespace: "epets_test", expires_in: 2.seconds }
   end
 
+  let(:client) { subject.dalli }
+  let(:exception) { Dalli::DalliError.new }
+
+  let(:ttl_key) { "epets_test:foo.ttl" }
+  let(:ttl_set_args) { [ttl_key, "", 2.seconds, raw: true] }
+  let(:ttl_get_args) { [ttl_key, raw: true] }
+  let(:ttl_add_args) { [ttl_key, "", 10, raw: true] }
+
   around do |example|
-    subject.with do |client|
-      @client = client
-      @client.delete("epets_test:foo")
-      @client.delete("epets_test:foo.ttl")
+    client.delete("epets_test:foo")
+    client.delete("epets_test:foo.ttl")
+    example.run
+  end
 
-      example.run
-    end
+  before do
+    allow(client).to receive(:get).and_call_original
+    allow(client).to receive(:set).and_call_original
+    allow(client).to receive(:add).and_call_original
+    allow(client).to receive(:delete).and_call_original
   end
 
   describe "#fetch" do
@@ -27,21 +38,26 @@
         expect {
           subject.fetch("foo", options) { "bar" }
         }.to change {
-          @client.get("epets_test:foo")
+          client.get("epets_test:foo")
         }.from(nil).to("bar")
       end
 
       it "writes the TTL value to the cache" do
         expect {
           subject.fetch("foo", options) { "bar" }
         }.to change {
-          @client.get("epets_test:foo.ttl")
+          client.get("epets_test:foo.ttl")
         }.from(nil).to("")
       end
 
       it "returns the value" do
         expect(subject.fetch("foo", options) { "bar" }).to eq("bar")
       end
+
+      it "handles exceptions" do
+        expect(subject.dalli).to receive(:set).with(*ttl_set_args).and_raise(exception)
+        expect(subject.fetch("foo", options) { "bar" }).to eq("bar")
+      end
     end
 
     context "when the cache is set" do
@@ -58,13 +74,24 @@
       it "returns the value" do
         expect(subject.fetch("foo", options) { "bar" }).to eq("bar")
       end
+
+      it "handles exceptions when reading the lock" do
+        expect(subject.dalli).to receive(:get).with(*ttl_get_args).and_raise(exception)
+        expect(subject.fetch("foo", options) { "bar" }).to eq("bar")
+      end
+
+      it "handles exceptions when setting the lock" do
+        client.delete(ttl_key)
+        expect(subject.dalli).to receive(:add).with(*ttl_add_args).and_raise(exception)
+        expect(subject.fetch("foo", options) { "bar" }).to eq("bar")
+      end
     end
   end
 
   describe "#read" do
     context "when the cache is not set" do
       it "returns nil" do
-        expect(subject.read("foo")).to be_nil
+        expect(subject.read("foo", options)).to be_nil
       end
     end
 
@@ -76,6 +103,17 @@
       it "returns the value" do
         expect(subject.read("foo", options)).to eq("bar")
       end
+
+      it "handles exceptions when reading the lock" do
+        expect(subject.dalli).to receive(:get).with(*ttl_get_args).and_raise(exception)
+        expect(subject.read("foo", options)).to eq("bar")
+      end
+
+      it "handles exceptions when setting the lock" do
+        client.delete(ttl_key)
+        expect(subject.dalli).to receive(:add).with(*ttl_add_args).and_raise(exception)
+        expect(subject.read("foo", options)).to eq("bar")
+      end
     end
   end
 
@@ -84,17 +122,22 @@
       expect {
         subject.write("foo", "bar", options)
       }.to change {
-        @client.get("epets_test:foo")
+        client.get("epets_test:foo")
       }.from(nil).to("bar")
     end
 
     it "writes the TTL value to the cache" do
       expect {
         subject.write("foo", "bar", options)
       }.to change {
-        @client.get("epets_test:foo.ttl")
+        client.get("epets_test:foo.ttl")
       }.from(nil).to("")
     end
+
+    it "handles exceptions when setting the TTL" do
+      expect(subject.dalli).to receive(:set).with(*ttl_set_args).and_raise(exception)
+      expect(subject.write("foo", "bar", options)).to be_falsey
+    end
   end
 
   describe "#delete" do
@@ -106,16 +149,21 @@
       expect {
         subject.delete("foo", options)
       }.to change {
-        @client.get("epets_test:foo")
+        client.get("epets_test:foo")
       }.from("bar").to(nil)
     end
 
     it "deletes the TTL value from the cache" do
       expect {
         subject.delete("foo", options)
       }.to change {
-        @client.get("epets_test:foo.ttl")
+        client.get("epets_test:foo.ttl")
       }.from("").to(nil)
     end
+
+    it "handles exceptions when deleting the TTL" do
+      expect(subject.dalli).to receive(:delete).with(ttl_key).and_raise(exception)
+      expect(subject.delete("foo", options)).to be_falsey
+    end
   end
 end