Fix thread safeness issues

Signed-off-by: Ahmad Tolba <tolpa1@gmail.com>

lib/prometheus/client/registry.rb

@@ -1,11 +1,10 @@
 # encoding: UTF-8
 
-require 'thread'
-
 require 'prometheus/client/counter'
 require 'prometheus/client/summary'
 require 'prometheus/client/gauge'
 require 'prometheus/client/histogram'
+require 'concurrent'
 
 module Prometheus
   module Client
@@ -15,24 +14,23 @@ class AlreadyRegisteredError < StandardError; end
 
       def initialize
         @metrics = {}
-        @mutex = Mutex.new
+        @lock    = Concurrent::ReentrantReadWriteLock.new
       end
 
       def register(metric)
         name = metric.name
 
-        @mutex.synchronize do
-          if exist?(name.to_sym)
-            raise AlreadyRegisteredError, "#{name} has already been registered"
-          end
+        @lock.with_write_lock do
+          return metric if exist?(name.to_sym)
+
           @metrics[name.to_sym] = metric
         end
 
         metric
       end
 
       def unregister(name)
-        @mutex.synchronize do
+        @lock.with_write_lock do
           @metrics.delete(name.to_sym)
         end
       end
@@ -73,15 +71,21 @@ def histogram(name, docstring:, labels: [], preset_labels: {},
       end
 
       def exist?(name)
-        @metrics.key?(name)
+        @lock.with_read_lock do
+          @metrics.key?(name)
+        end
       end
 
       def get(name)
-        @metrics[name.to_sym]
+        @lock.with_read_lock do
+          @metrics[name.to_sym]
+        end
       end
 
       def metrics
-        @metrics.values
+        @lock.with_read_lock do
+          @metrics.values
+        end
       end
     end
   end

spec/prometheus/client/registry_spec.rb

@@ -19,36 +19,26 @@
       expect(registry.register(metric)).to eql(metric)
     end
 
-    it 'raises an exception if a metric name gets registered twice' do
+    it 'returns a metric if a metric name tries to register twice' do
       metric = double(name: :test)
 
       registry.register(metric)
 
-      expect do
-        registry.register(metric)
-      end.to raise_exception described_class::AlreadyRegisteredError
+      expect(registry.metrics.size).to eq 1
     end
 
     it 'is thread safe' do
-      mutex = Mutex.new
-      containers = []
-
       def registry.exist?(*args)
         super.tap { sleep(0.01) }
       end
 
       Array.new(5) do
         Thread.new do
-          result = begin
-            registry.register(double(name: :test))
-          rescue Prometheus::Client::Registry::AlreadyRegisteredError
-            nil
-          end
-          mutex.synchronize { containers << result }
+          registry.register(double(name: :test))
         end
       end.each(&:join)
 
-      expect(containers.compact.size).to eql(1)
+      expect(registry.metrics.size).to eql(1)
     end
   end
 

spec/prometheus/integration_spec.rb

@@ -28,4 +28,3 @@
     end
   end
 end
-

spec/prometheus/threaded_integration_spec.rb

@@ -0,0 +1,52 @@
+# encoding: UTF-8
+
+require 'rack/test'
+require 'rack'
+require 'prometheus/middleware/collector'
+require 'prometheus/middleware/exporter'
+require "concurrent"
+
+API = Rack::Builder.new do
+  use Rack::Deflater
+  use Prometheus::Middleware::Collector
+  use Prometheus::Middleware::Exporter
+
+  map "/" do
+    run ->(_) { [200, {'Content-Type' => 'text/html'}, ['OK']] }
+  end
+end
+
+describe API do
+  include Rack::Test::Methods
+
+  let(:app) { described_class }
+
+  context 'GET /metrics' do
+    it "fails when it's multi threaded request" do
+      latch = Concurrent::CountDownLatch.new(1)
+
+      t1 = Thread.new do
+        latch.wait
+        get '/metrics'
+      end
+
+      t2 = Thread.new do
+        latch.wait
+
+        get '/metrics'
+      end
+
+      t3 = Thread.new do
+        latch.wait
+
+        get '/metrics'
+      end
+
+      latch.count_down
+
+      [t1, t2, t3].each(&:join)
+
+      expect { last_response }.not_to raise_error
+    end
+  end
+end