Merge branch 'sh-add-rugged-cleanup' into 'master'

Add support for closing Rugged/libgit2 file descriptors

See merge request gitlab-org/gitaly!903

3 files changed, 70 insertions, 3 deletions

diff --git a/changelogs/unreleased/sh-add-rugged-cleanup.yml b/changelogs/unreleased/sh-add-rugged-cleanup.yml
new file mode 100644
index 000000000..a90c7de87
--- /dev/null
+++ b/changelogs/unreleased/sh-add-rugged-cleanup.yml
@@ -0,0 +1,5 @@
+---
+title: Add support for closing Rugged/libgit2 file descriptors
+merge_request: 903
+author:
+type: added
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index d6ea03041..fe68b45dd 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -48,6 +48,14 @@ module Gitlab
           )
         end
 
+        def from_gitaly_with_block(gitaly_repository, call)
+          repository = from_gitaly(gitaly_repository, call)
+
+          yield repository
+
+          repository.cleanup
+        end
+
         def create(repo_path)
           FileUtils.mkdir_p(repo_path, mode: 0770)
 
@@ -165,7 +173,7 @@ module Gitlab
       end
 
       def rugged
-        Rugged::Repository.new(path, alternates: alternate_object_directories)
+        @rugged ||= Rugged::Repository.new(path, alternates: alternate_object_directories)
       rescue Rugged::RepositoryError, Rugged::OSError
         raise NoRepository.new('no repository for such path')
       end
@@ -808,6 +816,12 @@ module Gitlab
         run_git!(args, lazy_block: block)
       end
 
+      def cleanup
+        # Opening a repository may be expensive, and we only need to close it
+        # if it's been open.
+        rugged&.close if defined?(@rugged)
+      end
+
       private
 
       def run_git(args, chdir: path, env: {}, nice: false, lazy_block: nil, &block)
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index a8592e08b..d78463ed0 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -1,10 +1,58 @@
 require 'spec_helper'
 
-describe Gitlab::Git::RevList do
+describe Gitlab::Git::Repository do
   include TestRepo
 
   let(:repository) { gitlab_git_from_gitaly(test_repo_read_only) }
 
+  describe '#from_gitaly_with_block' do
+    let(:call_metadata) do
+      {
+        'user-agent' => 'grpc-go/1.9.1',
+        'gitaly-storage-path' => DEFAULT_STORAGE_DIR,
+        'gitaly-repo-path' => TEST_REPO_PATH,
+        'gitaly-gl-repository' => 'project-52',
+        'gitaly-repo-alt-dirs' => ''
+      }
+    end
+    let(:call) { double(metadata: call_metadata) }
+    let(:gitlab_shell_path) { '/foo/bar/gitlab-shell' }
+
+    before do
+      ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] = gitlab_shell_path
+    end
+
+    after do
+      ENV.delete('GITALY_RUBY_GITLAB_SHELL_PATH')
+    end
+
+    it 'cleans up the repository' do
+      described_class.from_gitaly_with_block(test_repo_read_only, call) do |repository|
+        expect(repository.rugged).to receive(:close)
+      end
+    end
+  end
+
+  describe '#cleanup' do
+    context 'when Rugged has been called' do
+      it 'calls close on Rugged::Repository' do
+        rugged = repository.rugged
+
+        expect(rugged).to receive(:close).and_call_original
+
+        repository.cleanup
+      end
+    end
+
+    context 'when Rugged has not been called' do
+      it 'does not call close on Rugged::Repository' do
+        expect(repository).not_to receive(:rugged)
+
+        repository.cleanup
+      end
+    end
+  end
+
   describe '#parse_raw_diff_line' do
     let(:diff_data) { repository.parse_raw_diff_line(diff_line) }
 
@@ -36,7 +84,7 @@ describe Gitlab::Git::RevList do
       let(:diff_line) { '' }
 
       it 'raises an ArgumentError' do
-        expect {diff_data }.to raise_error(ArgumentError)
+        expect { diff_data }.to raise_error(ArgumentError)
       end
     end
   end