Get rid of lock cleanup

lib/stoplight/data_store/base.rb

@@ -54,16 +54,11 @@ def clear_state(_light)
       end
 
       # @param _light [Light]
+      # @param _from_color [String]
+      # @param _to_color [String]
       # @yield _block
       # @return [Void]
-      def with_notification_lock(_light, &_block)
-        raise NotImplementedError
-      end
-
-      # @param _light [Light]
-      # @yield _block
-      # @return [Void]
-      def with_lock_cleanup(_light, &_block)
+      def with_notification_lock(_light, _from_color, _to_color, &_block)
         raise NotImplementedError
       end
     end

lib/stoplight/data_store/memory.rb

@@ -7,12 +7,12 @@ module DataStore
     # @see Base
     class Memory < Base
       include MonitorMixin
-      LOCKED_STATUS = 1
+      KEY_SEPARATOR = ':'
 
       def initialize
         @failures = Hash.new { |h, k| h[k] = [] }
         @states = Hash.new { |h, k| h[k] = State::UNLOCKED }
-        @notification_locks = {}
+        @last_notifications = {}
         super() # MonitorMixin
       end
 
@@ -52,20 +52,28 @@ def clear_state(light)
         synchronize { @states.delete(light.name) }
       end
 
-      def with_notification_lock(light)
+      def with_notification_lock(light, from_color, to_color)
         synchronize do
-          return if @notification_locks[light.name] == LOCKED_STATUS
+          if last_notification(light) != [from_color, to_color]
+            set_last_notification(light, from_color, to_color)
 
-          @notification_locks[light.name] = LOCKED_STATUS
-          yield
+            yield
+          end
         end
       end
 
-      def with_lock_cleanup(light)
-        synchronize do
-          @notification_locks.delete(light.name)
-          yield
-        end
+      # @param light [Stoplight::Light]
+      # @return [Array, nil]
+      def last_notification(light)
+        @last_notifications[light.name]
+      end
+
+      # @param light [Stoplight::Light]
+      # @param from_color [String]
+      # @param to_color [String]
+      # @return [void]
+      def set_last_notification(light, from_color, to_color)
+        @last_notifications[light.name] = [from_color, to_color]
       end
     end
   end

lib/stoplight/data_store/redis.rb

@@ -79,18 +79,34 @@ def clear_state(light)
         normalize_state(state)
       end
 
-      def with_notification_lock(light)
-        yield if @redlock.lock(notification_lock_key(light), 1_000)
-      end
+      def with_notification_lock(light, from_color, to_color)
+        @redlock.lock(notification_lock_key(light), 2_000) do |acquired|
+          next unless acquired
 
-      def with_lock_cleanup(light)
-        @redlock.unlock(notification_lock_key(light))
+          if last_notification(light) != [from_color, to_color]
+            set_last_notification(light, from_color, to_color)
 
-        yield
+            yield
+          end
+        end
       end
 
       private
 
+      # @param light [Stoplight::Light]
+      # @return [Array, nil]
+      def last_notification(light)
+        @redis.get(last_notification_key(light))&.split('->')
+      end
+
+      # @param light [Stoplight::Light]
+      # @param from_color [String]
+      # @param to_color [String]
+      # @return [void]
+      def set_last_notification(light, from_color, to_color)
+        @redis.set(last_notification_key(light), [from_color, to_color].join('->'))
+      end
+
       def query_failures(light, transaction: @redis)
         transaction.lrange(failures_key(light), 0, -1)
       end
@@ -120,6 +136,10 @@ def notification_lock_key(light)
         key('notification_lock', light.name)
       end
 
+      def last_notification_key(light)
+        key('last_notification', light.name)
+      end
+
       def states_key
         key('states')
       end

lib/stoplight/light/runnable.rb

@@ -30,7 +30,7 @@ def run
 
       def run_green
         on_failure = lambda do |size, error|
-          execute_once { notify(Color::GREEN, Color::RED, error) } if failures_threshold_breached?(size, threshold)
+          notify(Color::GREEN, Color::RED, error) if failures_threshold_breached?(size, threshold)
         end
         run_code(nil, on_failure)
       end
@@ -41,7 +41,7 @@ def failures_threshold_breached?(current_failures_count, max_errors_threshold)
 
       def run_yellow
         on_success = lambda do |failures|
-          with_cleanup { notify(Color::RED, Color::GREEN) } unless failures.empty?
+          notify(Color::RED, Color::GREEN) unless failures.empty?
         end
         run_code(on_success, nil)
       end
@@ -70,14 +70,6 @@ def handle_error(error, on_failure)
         fallback.call(error)
       end
 
-      def execute_once(&block)
-        safely { data_store.with_notification_lock(self) { block.call } }
-      end
-
-      def with_cleanup(&block)
-        safely { data_store.with_lock_cleanup(self) { block.call } }
-      end
-
       def clear_failures
         safely([]) { data_store.clear_failures(self) }
       end
@@ -87,8 +79,10 @@ def failures_and_state
       end
 
       def notify(from_color, to_color, error = nil)
-        notifiers.each do |notifier|
-          safely { notifier.notify(self, from_color, to_color, error) }
+        data_store.with_notification_lock(self, from_color, to_color) do
+          notifiers.each do |notifier|
+            safely { notifier.notify(self, from_color, to_color, error) }
+          end
         end
       end
 

spec/stoplight/data_store/base_spec.rb

@@ -64,14 +64,7 @@
 
   describe '#with_notification_lock' do
     it 'is not implemented' do
-      expect { data_store.with_notification_lock(nil) }
-        .to raise_error(NotImplementedError)
-    end
-  end
-
-  describe '#with_lock_cleanup' do
-    it 'is not implemented' do
-      expect { data_store.with_lock_cleanup(nil) }
+      expect { data_store.with_notification_lock(nil, nil, nil) }
         .to raise_error(NotImplementedError)
     end
   end

spec/stoplight/data_store/memory_spec.rb

@@ -132,30 +132,28 @@
   end
 
   describe '#with_notification_lock' do
-    context 'notification lock was not yet set' do
-      it 'yields passed block' do
-        expect { |b| data_store.with_notification_lock(light, &b) }.to yield_control
+    context 'when notification is already sent' do
+      before do
+        data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED) {}
       end
-    end
-
-    context 'notification lock was already set' do
-      before { data_store.with_notification_lock(light) {} }
 
       it 'does not yield passed block' do
-        expect { |b| data_store.with_notification_lock(light, &b) }.to_not yield_control
+        expect do |b|
+          data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED, &b)
+        end.not_to yield_control
       end
     end
-  end
-
-  describe '#with_lock_cleanup' do
-    it 'removes notification lock' do
-      data_store.with_lock_cleanup(light) {}
 
-      expect { |b| data_store.with_notification_lock(light, &b) }.to yield_control
-    end
+    context 'when notification is not already sent' do
+      before do
+        data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED) {}
+      end
 
-    it 'yields passed block' do
-      expect { |b| data_store.with_lock_cleanup(light, &b) }.to yield_control
+      it 'yields passed block' do
+        expect do |b|
+          data_store.with_notification_lock(light, Stoplight::Color::RED, Stoplight::Color::GREEN, &b)
+        end.to yield_control
+      end
     end
   end
 end

spec/stoplight/data_store/redis_spec.rb

@@ -146,30 +146,56 @@
   describe '#with_notification_lock' do
     let(:lock_key) { "stoplight:notification_lock:#{name}" }
 
-    context 'notification lock was not yet set' do
-      it 'yields passed block' do
-        expect(redlock).to receive(:lock).with(lock_key, 1_000).and_return({})
+    context 'notification lock is already acquired by another participant' do
+      it 'does not yield passed block' do
+        expect(redlock).to receive(:lock).with(lock_key, 2_000).and_yield(false)
 
-        expect { |b| data_store.with_notification_lock(light, &b) }.to yield_control
+        expect do |b|
+          data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED, &b)
+        end.not_to yield_control
       end
     end
 
-    context 'notification lock was already set' do
-      it 'does not yield passed block' do
-        expect(redlock).to receive(:lock).with(lock_key, 1_000).and_return(false)
+    context 'notification lock is not yet acquired by another participant' do
+      before do
+        allow(redlock).to receive(:lock).with(lock_key, 2_000).and_yield(true)
+      end
+
+      context 'when notification is already sent' do
+        before do
+          data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED) {}
+        end
 
-        expect { |b| data_store.with_notification_lock(light, &b) }.to_not yield_control
+        it 'does not yield passed block' do
+          expect do |b|
+            data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED, &b)
+          end.not_to yield_control
+        end
       end
-    end
-  end
 
-  describe '#with_lock_cleanup' do
-    let(:lock_key) { "stoplight:notification_lock:#{name}" }
+      context 'when notification is not already sent' do
+        before do
+          data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED) {}
+        end
 
-    it 'yields passed block' do
-      expect(redlock).to receive(:unlock).with(lock_key)
+        it 'yields passed block' do
+          expect do |b|
+            data_store.with_notification_lock(light, Stoplight::Color::RED, Stoplight::Color::GREEN, &b)
+          end.to yield_control
+        end
+      end
+    end
+
+    context 'notification lock is already acquired by another participant' do
+      before do
+        allow(redlock).to receive(:lock).with(lock_key, 2_000).and_yield(false)
+      end
 
-      expect { |b| data_store.with_lock_cleanup(light, &b) }.to yield_control
+      it 'does not yield passed block' do
+        expect do |b|
+          data_store.with_notification_lock(light, Stoplight::Color::GREEN, Stoplight::Color::RED, &b)
+        end.not_to yield_control
+      end
     end
   end
 end

spec/stoplight/light/runnable_spec.rb

@@ -109,36 +109,6 @@ def random_string
           expect(subject.data_store.get_failures(subject).size).to eql(1)
         end
 
-        context 'when we did not send notifications yet' do
-          it 'notifies when transitioning to red' do
-            subject.threshold.times do
-              expect(io.string).to eql('')
-              begin
-                subject.run
-              rescue error.class
-                nil
-              end
-            end
-            expect(io.string).to_not eql('')
-          end
-        end
-
-        context 'when we already sent notifications' do
-          before { subject.data_store.with_notification_lock(subject) {} }
-
-          it 'does not send new notifications' do
-            subject.threshold.times do
-              expect(io.string).to eql('')
-              begin
-                subject.run
-              rescue error.class
-                nil
-              end
-            end
-            expect(io.string).to eql('')
-          end
-        end
-
         it 'notifies when transitioning to red' do
           subject.threshold.times do
             expect(io.string).to eql('')
@@ -239,12 +209,6 @@ def random_string
         expect(subject.run).to eql(code_result)
       end
 
-      it 'performs notification locks cleanup' do
-        expect(subject.data_store).to receive(:with_lock_cleanup).with(subject)
-
-        subject.run
-      end
-
       it 'notifies when transitioning to green' do
         expect(io.string).to eql('')
         subject.run