Free Rugged open file descriptors in gRPC middleware

Close unneeded file descriptors and free memory in libgit2 without waiting for
Ruby garbage collection.

Closes #1359

6 files changed, 105 insertions, 2 deletions

diff --git a/changelogs/unreleased/sh-rugged-cleanup-gprc-middleware.yml b/changelogs/unreleased/sh-rugged-cleanup-gprc-middleware.yml
new file mode 100644
index 000000000..667c1304e
--- /dev/null
+++ b/changelogs/unreleased/sh-rugged-cleanup-gprc-middleware.yml
@@ -0,0 +1,5 @@
+---
+title: Free Rugged open file descriptors in gRPC middleware
+merge_request: 911
+author:
+type: performance
diff --git a/ruby/bin/gitaly-ruby b/ruby/bin/gitaly-ruby
index 2b85af7fd..647bbcd89 100755
--- a/ruby/bin/gitaly-ruby
+++ b/ruby/bin/gitaly-ruby
@@ -7,6 +7,7 @@ require 'grpc'
 require_relative '../lib/gitaly_server.rb'
 require_relative '../lib/gitaly_server/sentry_interceptor.rb'
 require_relative '../lib/gitaly_server/exception_sanitizer_interceptor.rb'
+require_relative '../lib/gitaly_server/rugged_interceptor.rb'
 
 SHUTDOWN_TIMEOUT = 600
 
@@ -28,7 +29,8 @@ def main
     poll_period: SHUTDOWN_TIMEOUT,
     interceptors: [
       GitalyServer::SentryInterceptor.new,
-      GitalyServer::ExceptionSanitizerInterceptor.new
+      GitalyServer::ExceptionSanitizerInterceptor.new,
+      GitalyServer::RuggedInterceptor.new
     ]
   )
   port = 'unix:' + socket_path
diff --git a/ruby/lib/gitaly_server/rugged_interceptor.rb b/ruby/lib/gitaly_server/rugged_interceptor.rb
new file mode 100644
index 000000000..574d4ccb9
--- /dev/null
+++ b/ruby/lib/gitaly_server/rugged_interceptor.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'grpc'
+
+module GitalyServer
+  class RuggedInterceptor < GRPC::ServerInterceptor
+    RUGGED_KEY = :rugged_list
+
+    # Intercept a unary request response call
+    %i[request_response server_streamer client_streamer bidi_streamer].each do |meth|
+      define_method(meth) do |**, &blk|
+        init_rugged_reference_list
+
+        blk.call
+
+        cleanup_rugged_references
+      end
+    end
+
+    def init_rugged_reference_list
+      Thread.current[RUGGED_KEY] = []
+    end
+
+    def cleanup_rugged_references
+      repos = Thread.current[RUGGED_KEY]
+      repos.compact.map(&:close)
+    end
+  end
+end
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index fe68b45dd..6e0351d45 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -26,6 +26,7 @@ module Gitlab
       GITLAB_PROJECTS_TIMEOUT = Gitlab.config.gitlab_shell.git_timeout
       EMPTY_REPOSITORY_CHECKSUM = '0000000000000000000000000000000000000000'.freeze
       AUTOCRLF_VALUES = { 'true' => true, 'false' => false, 'input' => :input }.freeze
+      RUGGED_KEY = :rugged_list
 
       NoRepository = Class.new(StandardError)
       InvalidRepository = Class.new(StandardError)
@@ -173,7 +174,11 @@ module Gitlab
       end
 
       def rugged
-        @rugged ||= Rugged::Repository.new(path, alternates: alternate_object_directories)
+        @rugged ||= begin
+                      Rugged::Repository.new(path, alternates: alternate_object_directories).tap do |repo|
+                        Thread.current[RUGGED_KEY] << repo if Thread.current[RUGGED_KEY]
+                      end
+                    end
       rescue Rugged::RepositoryError, Rugged::OSError
         raise NoRepository.new('no repository for such path')
       end
diff --git a/ruby/spec/lib/gitaly_server/rugged_interceptor_spec.rb b/ruby/spec/lib/gitaly_server/rugged_interceptor_spec.rb
new file mode 100644
index 000000000..0d1cb9eaf
--- /dev/null
+++ b/ruby/spec/lib/gitaly_server/rugged_interceptor_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+require_relative '../../../lib/gitaly_server/rugged_interceptor.rb'
+
+describe GitalyServer::RuggedInterceptor do
+  include TestRepo
+
+  let(:meth) { GitalyServer::RefService.instance_method(:create_branch) }
+  let(:call) { double(metadata: {}) }
+
+  subject do
+    described_class.new.server_streamer(call: call, method: meth) { }
+  end
+
+  context 'no Rugged repositories initialized' do
+    it 'does not clean up any repositories' do
+      expect(Rugged::Repository).not_to receive(:new)
+
+      subject
+    end
+  end
+
+  context 'Rugged repository initialized' do
+    let(:rugged) { rugged_from_gitaly(test_repo_read_only) }
+
+    let(:streamer) do
+      described_class.new.server_streamer(call: call, method: meth) do
+        Thread.current[GitalyServer::RuggedInterceptor::RUGGED_KEY] = [rugged]
+      end
+    end
+
+    it 'cleans up repositories' do
+      expect(rugged).to receive(:close).and_call_original
+
+      streamer
+    end
+  end
+end
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index d78463ed0..f7464b837 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -53,6 +53,30 @@ describe Gitlab::Git::Repository do
     end
   end
 
+  describe '#rugged' do
+    after do
+      Thread.current[described_class::RUGGED_KEY] = nil
+    end
+
+    it 'stores reference in Thread.current' do
+      Thread.current[described_class::RUGGED_KEY] = []
+
+      2.times do
+        rugged = repository.rugged
+
+        expect(rugged).to be_a(Rugged::Repository)
+        expect(Thread.current[described_class::RUGGED_KEY]).to eq([rugged])
+      end
+    end
+
+    it 'does not store reference if Thread.current is not set up' do
+      rugged = repository.rugged
+
+      expect(rugged).to be_a(Rugged::Repository)
+      expect(Thread.current[described_class::RUGGED_KEY]).to be_nil
+    end
+  end
+
   describe '#parse_raw_diff_line' do
     let(:diff_data) { repository.parse_raw_diff_line(diff_line) }