Don't reuse gRPC channels

It seems that bad things happen when two gRPC stubs share one gRPC
channel so let's stop doing that. The downside of this is that we
create more gRPC connections; one per stub.

app/models/repository.rb

@@ -1163,8 +1163,6 @@ class Repository
     @project.repository_storage_path
   end
 
-  delegate :gitaly_channel, :gitaly_repository, to: :raw_repository
-
   def initialize_raw_repository
     Gitlab::Git::Repository.new(project.repository_storage, path_with_namespace + '.git')
   end

config/initializers/8_gitaly.rb

 require 'uri'
 
-# Make sure we initialize our Gitaly channels before Sidekiq starts multi-threaded execution.
 if Gitlab.config.gitaly.enabled || Rails.env.test?
-  Gitlab::GitalyClient.configure_channels
+  Gitlab.config.repositories.storages.keys.each do |storage|
+    # Force validation of each address
+    Gitlab::GitalyClient.address(storage)
+  end
 end

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

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?

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

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

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

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

spec/lib/gitlab/gitaly_client_spec.rb

 require 'spec_helper'
 
 describe Gitlab::GitalyClient, lib: true do
-  describe '.new_channel' do
+  describe '.stub' do
+    before { described_class.clear_stubs! }
+
     context 'when passed a UNIX socket address' do
-      it 'passes the address as-is to GRPC::Core::Channel initializer' do
+      it 'passes the address as-is to GRPC' do
         address = 'unix:/tmp/gitaly.sock'
+        allow(Gitlab.config.repositories).to receive(:storages).and_return({
+          'default' => { 'gitaly_address' => address }
+        })
 
-        expect(GRPC::Core::Channel).to receive(:new).with(address, any_args)
+        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
 
-        described_class.new_channel(address)
+        described_class.stub(:commit, 'default')
       end
     end
 
@@ -17,9 +22,13 @@ describe Gitlab::GitalyClient, lib: true do
         address = 'localhost:9876'
         prefixed_address = "tcp://#{address}"
 
-        expect(GRPC::Core::Channel).to receive(:new).with(address, any_args)
+        allow(Gitlab.config.repositories).to receive(:storages).and_return({
+          'default' => { 'gitaly_address' => prefixed_address }
+        })
+
+        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
 
-        described_class.new_channel(prefixed_address)
+        described_class.stub(:commit, 'default')
       end
     end
   end

spec/lib/gitlab/workhorse_spec.rb

@@ -202,7 +202,7 @@ describe Gitlab::Workhorse, lib: true do
     context 'when Gitaly is enabled' do
       let(:gitaly_params) do
         {
-          GitalyAddress: Gitlab::GitalyClient.get_address('default')
+          GitalyAddress: Gitlab::GitalyClient.address('default')
         }
       end
 

spec/support/test_env.rb

@@ -120,7 +120,7 @@ module TestEnv
   end
 
   def setup_gitaly
-    socket_path = Gitlab::GitalyClient.get_address('default').sub(/\Aunix:/, '')
+    socket_path = Gitlab::GitalyClient.address('default').sub(/\Aunix:/, '')
     gitaly_dir = File.dirname(socket_path)
 
     unless File.directory?(gitaly_dir) || system('rake', "gitlab:gitaly:install[#{gitaly_dir}]")

spec/tasks/gitlab/backup_rake_spec.rb

@@ -236,7 +236,6 @@ describe 'gitlab:app namespace rake task' do
           'custom' => { 'path' => Settings.absolute('tmp/tests/custom_storage'), 'gitaly_address' => gitaly_address }
         }
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
-        Gitlab::GitalyClient.configure_channels
 
         # Create the projects now, after mocking the settings but before doing the backup
         project_a