- use ActiveSupport::Notifications to instrument events

- fix an issue when getting an exception inside application would not delete lock, so client could receive 429 after 500

idempotent-request.gemspec

@@ -27,5 +27,5 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'rake', '~> 10.0'
   spec.add_development_dependency 'rspec', '~> 3.0'
   spec.add_development_dependency 'fakeredis', '~> 0.6'
-  spec.add_development_dependency 'pry', '~> 0.11'
+  spec.add_development_dependency 'byebug', '~> 10.0'
 end

lib/idempotent-request/middleware.rb

@@ -4,6 +4,7 @@ module IdempotentRequest
       @app = app
       @config = config
       @policy = config.fetch(:policy)
+      @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
     end
 
     def call(env)
@@ -13,10 +14,12 @@ module IdempotentRequest
 
     def process(env)
       set_request(env)
+      request.env['idempotent.request'] = {}
       return app.call(request.env) unless process?
-      read_idempotent_request ||
+      request.env['idempotent.request']['key'] = request.key
-        write_idempotent_request ||
+      response = read_idempotent_request || write_idempotent_request || concurrent_request_response
-        concurrent_request_response
+      instrument(request)
+      response
     end
 
     private
@@ -26,19 +29,30 @@ module IdempotentRequest
     end
 
     def read_idempotent_request
-      storage.read
+      request.env['idempotent.request']['read'] = storage.read
     end
 
     def write_idempotent_request
       return unless storage.lock
-      storage.write(*app.call(request.env))
+      begin
+        result = app.call(request.env)
+        request.env['idempotent.request']['write'] = result
+        storage.write(*result)
+      ensure
+        request.env['idempotent.request']['unlocked'] = storage.unlock
+        result
+      end
     end
 
     def concurrent_request_response
-      [429, {}, []]
+      status = 429
+      headers = { 'Content-Type' => 'application/json' }
+      body = [ Oj.dump('error' => 'Concurrent requests detected') ]
+      request.env['idempotent.request']['concurrent_request_response'] = true
+      Rack::Response.new(body, status, headers).finish
     end
 
-    attr_reader :app, :env, :config, :request, :policy
+    attr_reader :app, :env, :config, :request, :policy, :notifier
 
     def process?
       !request.key.to_s.empty? && should_be_idempotent?
@@ -49,6 +63,10 @@ module IdempotentRequest
       policy.new(request).should?
     end
 
+    def instrument(request)
+      notifier.instrument('idempotent.request', request: request) if notifier
+    end
+
     def set_request(env)
       @env = env
       @request ||= Request.new(env, config)

lib/idempotent-request/redis_storage.rb

@@ -9,7 +9,7 @@ module IdempotentRequest
     end
 
     def lock(key)
-      setnx_with_expiration(lock_key(key), true)
+      setnx_with_expiration(lock_key(key), Time.now.to_f)
     end
 
     def unlock(key)

lib/idempotent-request/request.rb

@@ -22,10 +22,9 @@ module IdempotentRequest
     private
 
     def header_name
-      key = @header_name
+      key = @header_name.to_s
-        .to_s
                         .upcase
-        .gsub('-', '_')
+                        .tr('-', '_')
 
       key.start_with?('HTTP_') ? key : "HTTP_#{key}"
     end

lib/idempotent-request/request_manager.rb

@@ -30,8 +30,6 @@ module IdempotentRequest
 
       if (200..226).cover?(status)
         storage.write(key, payload(status, headers, response))
-      else
-        unlock
       end
 
       data
@@ -46,11 +44,9 @@ module IdempotentRequest
     end
 
     def payload(status, headers, response)
-      Oj.dump({
+      Oj.dump(status: status,
-        status: status,
               headers: headers.to_h,
-        response: Array(response)
+              response: Array(response))
-      })
     end
 
     def run_callback(action, args)

spec/idempotent-request/middleware_spec.rb

@@ -29,6 +29,26 @@ RSpec.describe IdempotentRequest::Middleware do
       middleware.call(env)
     end
 
+    it 'should obtain lock and release lock' do
+      expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:lock).and_return(true)
+      expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:write)
+      expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:unlock)
+
+      middleware.call(env)
+    end
+
+    context 'when an exception happens inside another middleware' do
+      let(:app) { ->(_) { raise 'fatality' } }
+
+      it 'should release lock' do
+        expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:lock).and_return(true)
+        expect_any_instance_of(IdempotentRequest::RequestManager).not_to receive(:write)
+        expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:unlock)
+
+        expect { middleware.call(env) }.to raise_error('fatality')
+      end
+    end
+
     context 'when has data in storage' do
       before do
         data = [200, {}, 'body']

spec/idempotent-request/redis_storage_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe IdempotentRequest::RedisStorage do
     let(:lock_key) { "#{namespace}:lock:#{key}" }
 
     it 'should add lock' do
-      expect(redis).to receive(:set).with(lock_key, true, nx: true, ex: expire_time)
+      expect(redis).to receive(:set).with(lock_key, Float, nx: true, ex: expire_time)
       redis_storage.lock(key)
     end
   end

spec/idempotent-request/request_manager_spec.rb

@@ -144,11 +144,6 @@ RSpec.describe IdempotentRequest::RequestManager do
         request_storage.write(*data)
         expect(memory_storage.read(request.key)).to be_nil
       end
-
-      it 'should unlock stored key' do
-        expect(memory_storage).to receive(:unlock).with(request.key)
-        request_storage.write(*data)
-      end
     end
   end
 

spec/spec_helper.rb

@@ -1,6 +1,6 @@
 require "bundler/setup"
 require 'fakeredis'
-require 'pry'
+require 'byebug'
 require "idempotent-request"
 
 spec = File.expand_path('../', __FILE__)