Merge branch 'gitaly-reuse-stubs' into 'master'

Reuse gRPC stubs instead of channels

Closes #31991 and gitaly#187

See merge request !11244

6 files changed, 36 insertions, 52 deletions

diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 256318cb833..7a051444603 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -27,13 +27,15 @@ module Gitlab
       # Rugged repo object
       attr_reader :rugged
 
+      attr_reader :storage
+
       # 'path' must be the path to a _bare_ git repository, e.g.
       # /path/to/my-repo.git
-      def initialize(repository_storage, relative_path)
-        @repository_storage = repository_storage
+      def initialize(storage, relative_path)
+        @storage = storage
         @relative_path = relative_path
 
-        storage_path = Gitlab.config.repositories.storages[@repository_storage]['path']
+        storage_path = Gitlab.config.repositories.storages[@storage]['path']
         @path = File.join(storage_path, @relative_path)
         @name = @relative_path.split("/").last
         @attributes = Gitlab::Git::Attributes.new(path)
@@ -965,11 +967,7 @@ module Gitlab
       end
 
       def gitaly_repository
-        Gitlab::GitalyClient::Util.repository(@repository_storage, @relative_path)
-      end
-
-      def gitaly_channel
-        Gitlab::GitalyClient.get_channel(@repository_storage)
+        Gitlab::GitalyClient::Util.repository(@storage, @relative_path)
       end
 
       private
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index c69676a1dac..72466700c05 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -4,56 +4,42 @@ module Gitlab
   module GitalyClient
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
 
-    # This function is not thread-safe because it updates Hashes in instance variables.
-    def self.configure_channels
-      @addresses = {}
-      @channels = {}
-      Gitlab.config.repositories.storages.each do |name, params|
-        address = params['gitaly_address']
-        unless address.present?
-          raise "storage #{name.inspect} is missing a gitaly_address"
-        end
+    MUTEX = Mutex.new
+    private_constant :MUTEX
 
-        unless URI(address).scheme.in?(%w(tcp unix))
-          raise "Unsupported Gitaly address: #{address.inspect} does not use URL scheme 'tcp' or 'unix'"
+    def self.stub(name, storage)
+      MUTEX.synchronize do
+        @stubs ||= {}
+        @stubs[storage] ||= {}
+        @stubs[storage][name] ||= begin
+          klass = Gitaly.const_get(name.to_s.camelcase.to_sym).const_get(:Stub)
+          addr = address(storage)
+          addr = addr.sub(%r{^tcp://}, '') if URI(addr).scheme == 'tcp'
+          klass.new(addr, :this_channel_is_insecure)
         end
-
-        @addresses[name] = address
-        @channels[name] = new_channel(address)
       end
     end
 
-    def self.new_channel(address)
-      address = address.sub(%r{^tcp://}, '') if URI(address).scheme == 'tcp'
-      # NOTE: When Gitaly runs on a Unix socket, permissions are
-      # handled using the file system and no additional authentication is
-      # required (therefore the :this_channel_is_insecure flag)
-      # TODO: Add authentication support when Gitaly is running on a TCP socket.
-      GRPC::Core::Channel.new(address, {}, :this_channel_is_insecure)
+    def self.clear_stubs!
+      MUTEX.synchronize do
+        @stubs = nil
+      end
     end
 
-    def self.get_channel(storage)
-      if !Rails.env.production? && @channels.nil?
-        # In development mode the Rails auto-loader may reset the instance
-        # variables of this class. What we do here is not thread-safe. In normal
-        # circumstances (including production) these instance variables have
-        # been initialized from config/initializers.
-        configure_channels
-      end
+    def self.address(storage)
+      params = Gitlab.config.repositories.storages[storage]
+      raise "storage not found: #{storage.inspect}" if params.nil?
 
-      @channels[storage]
-    end
+      address = params['gitaly_address']
+      unless address.present?
+        raise "storage #{storage.inspect} is missing a gitaly_address"
+      end
 
-    def self.get_address(storage)
-      if !Rails.env.production? && @addresses.nil?
-        # In development mode the Rails auto-loader may reset the instance
-        # variables of this class. What we do here is not thread-safe. In normal
-        # circumstances (including development) these instance variables have
-        # been initialized from config/initializers.
-        configure_channels
+      unless URI(address).scheme.in?(%w(tcp unix))
+        raise "Unsupported Gitaly address: #{address.inspect} does not use URL scheme 'tcp' or 'unix'"
       end
 
-      @addresses[storage]
+      address
     end
 
     def self.enabled?
diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit.rb
index 15c57420fb2..4491903d788 100644
--- a/lib/gitlab/gitaly_client/commit.rb
+++ b/lib/gitlab/gitaly_client/commit.rb
@@ -11,7 +11,7 @@ module Gitlab
       end
 
       def is_ancestor(ancestor_id, child_id)
-        stub = Gitaly::Commit::Stub.new(nil, nil, channel_override: @repository.gitaly_channel)
+        stub = GitalyClient.stub(:commit, @repository.storage)
         request = Gitaly::CommitIsAncestorRequest.new(
           repository: @gitaly_repo,
           ancestor_id: ancestor_id,
@@ -52,7 +52,7 @@ module Gitlab
       end
 
       def diff_service_stub
-        Gitaly::Diff::Stub.new(nil, nil, channel_override: @repository.gitaly_channel)
+        GitalyClient.stub(:diff, @repository.storage)
       end
     end
   end
diff --git a/lib/gitlab/gitaly_client/notifications.rb b/lib/gitlab/gitaly_client/notifications.rb
index a94a54883db..719554eac52 100644
--- a/lib/gitlab/gitaly_client/notifications.rb
+++ b/lib/gitlab/gitaly_client/notifications.rb
@@ -6,7 +6,7 @@ module Gitlab
       # 'repository' is a Gitlab::Git::Repository
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
-        @stub = Gitaly::Notifications::Stub.new(nil, nil, channel_override: repository.gitaly_channel)
+        @stub = GitalyClient.stub(:notifications, repository.storage)
       end
 
       def post_receive
diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref.rb
index f6c77ef1a3e..bf04e1fa50b 100644
--- a/lib/gitlab/gitaly_client/ref.rb
+++ b/lib/gitlab/gitaly_client/ref.rb
@@ -6,7 +6,7 @@ module Gitlab
       # 'repository' is a Gitlab::Git::Repository
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
-        @stub = Gitaly::Ref::Stub.new(nil, nil, channel_override: repository.gitaly_channel)
+        @stub = GitalyClient.stub(:ref, repository.storage)
       end
 
       def default_branch_name
diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index 351e2b10595..fe37e4da94f 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -26,7 +26,7 @@ module Gitlab
         }
 
         if Gitlab.config.gitaly.enabled
-          address = Gitlab::GitalyClient.get_address(project.repository_storage)
+          address = Gitlab::GitalyClient.address(project.repository_storage)
           params[:Repository] = repository.gitaly_repository.to_h
 
           feature_enabled = case action.to_s