Skip to content

Make sure on reset we release file descriptors for open files

vitiokss requested to merge vitiokss/prometheus-client-mmap:master into master

When using client Prometheus::Client.reset! method we are not releasing File descriptors for the mmap file and file lock. In this case (if using reset! in tests, to get a fresh instance of Prometheus client), quite quickly you are reaching too many open files exception.

To reproduce this, you can use the following script:

#!/usr/bin/env ruby

$LOAD_PATH << File.expand_path('../../lib', __FILE__)
require 'prometheus/client'

def fd_count
  puts "Number of open files:"
  puts `lsof -a -p #{Process.pid} | wc -l`
  puts "====================="
end

prometheus = Prometheus::Client.registry

http_requests = prometheus.counter(:http_requests, 'A counter of HTTP requests made')
http_requests.increment

fd_count

100.times do
  Prometheus::Client.reset!
end

fd_count

Before the fix (problem):

Number of open files:
      26
=====================
Number of open files:
     126
=====================

After the fix results:

Number of open files:
      26
=====================
Number of open files:
      26
=====================

The fix is made in two parts, one in MMAP C implementation, when unmap is triggered we also need to release File descriptor. Second, we never release file lock (Prometheus::Client::Helper::FileLock.unlock(path)). So every other reset we create a new file instead of sometimes re-using the same one (https://gitlab.com/gitlab-org/prometheus-client-mmap/-/blob/master/lib/prometheus/client/helper/mmaped_file.rb#L42)

Also for fresh reset, we need to wipe metric files. This behavior can be changed, if we instead of appending binary, would recreate the file each time we open it. I didn't go with this approach for now, as several tests will need to be reworked and bigger input would be needed.

Edited by vitiokss

Merge request reports

Loading