Skip to content

Commit

Permalink
Make all registry methods thread safe
Browse files Browse the repository at this point in the history
The registry is backed by a Hash, which is not guaranteed to be thread
safe on all interpreters. For peace of mind, this change synchronizes
all accesses to the metrics hash.

Another option would have been to use a [thread-safe Hash][1] instead of
a Hash but this would have meant adding Ruby Concurrent as a
dependency, which I'm assuming we don't want.

Ref: #184 (comment)

[1]: https://github.com/ruby-concurrency/concurrent-ruby/blob/v1.1.7/lib/concurrent-ruby/concurrent/hash.rb

Signed-off-by: Matthieu Prat <matthieuprat@gocardless.com>
  • Loading branch information
matthieuprat committed Aug 25, 2020
1 parent 872a8eb commit c3836b2
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 8 deletions.
8 changes: 4 additions & 4 deletions lib/prometheus/client/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def register(metric)
name = metric.name

@mutex.synchronize do
if exist?(name.to_sym)
if @metrics.key?(name.to_sym)
raise AlreadyRegisteredError, "#{name} has already been registered"
end
@metrics[name.to_sym] = metric
Expand Down Expand Up @@ -73,15 +73,15 @@ def histogram(name, docstring:, labels: [], preset_labels: {},
end

def exist?(name)
@metrics.key?(name)
@mutex.synchronize { @metrics.key?(name) }
end

def get(name)
@metrics[name.to_sym]
@mutex.synchronize { @metrics[name.to_sym] }
end

def metrics
@metrics.values
@mutex.synchronize { @metrics.values }
end
end
end
Expand Down
4 changes: 0 additions & 4 deletions spec/prometheus/client/registry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@
mutex = Mutex.new
containers = []

def registry.exist?(*args)
super.tap { sleep(0.01) }
end

Array.new(5) do
Thread.new do
result = begin
Expand Down

0 comments on commit c3836b2

Please sign in to comment.