Remove ruby concurrency limiter

6 files changed, 3 insertions, 82 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index aad074faa..776c892c8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 UNRELEASED
 
+- Remove ruby concurrency limiter
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/708
 - Introduce src-d/go-git as dependency
   https://gitlab.com/gitlab-org/gitaly/merge_requests/709
 
diff --git a/doc/configuration/README.md b/doc/configuration/README.md
index e685d52c9..88ac51b1e 100644
--- a/doc/configuration/README.md
+++ b/doc/configuration/README.md
@@ -112,7 +112,6 @@ max\_rss limit.
 |restart_delay|string|no|Time memory must be high before a restart is triggered, in seconds. Default 5 minutes ("5m").|
 |num_workers|integer|no|Number of gitaly-ruby worker processes. Default 2, minimum 2.|
 |linguist_languages_path|string|no|Override for dynamic languages.json discovery. Default: "" (use dynamic discovery).|
-|concurrency|integer|no|Maximum number of concurrent gitaly-ruby requests. Default 20.|
 
 ### Logging
 
diff --git a/internal/config/ruby.go b/internal/config/ruby.go
index 384ab5139..9ec0fd3b0 100644
--- a/internal/config/ruby.go
+++ b/internal/config/ruby.go
@@ -15,7 +15,6 @@ type Ruby struct {
 	RestartDelayToml           duration `toml:"restart_delay"`
 	NumWorkers                 int      `toml:"num_workers"`
 	LinguistLanguagesPath      string   `toml:"linguist_languages_path"`
-	Concurrency                int      `toml:"concurrency"`
 }
 
 // This type is a trick to let our TOML library parse durations from strings.
@@ -54,12 +53,5 @@ func ConfigureRuby() error {
 		Config.Ruby.NumWorkers = minWorkers
 	}
 
-	if Config.Ruby.Concurrency == 0 {
-		// You can benchmark this number with:
-		//
-		// go test -bench BenchmarkConcurrency -run '^$' ./internal/rubyserver/
-		Config.Ruby.Concurrency = 20
-	}
-
 	return validateIsDirectory(Config.Ruby.Dir, "gitaly-ruby.dir")
 }
diff --git a/internal/rubyserver/concurrency_test.go b/internal/rubyserver/concurrency_test.go
index 98b86f90a..e82311513 100644
--- a/internal/rubyserver/concurrency_test.go
+++ b/internal/rubyserver/concurrency_test.go
@@ -7,7 +7,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -15,42 +14,6 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-func TestConcurrency(t *testing.T) {
-	testhelper.ConfigureRuby()
-	config.Config.Ruby.Concurrency = 1
-
-	s, err := Start()
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer s.Stop()
-
-	require.NoError(t, waitPing(s), "wait for gitaly-ruby to boot")
-
-	start := make(chan struct{})
-	wg := &sync.WaitGroup{}
-
-	for i := 0; i < 100; i++ {
-		wg.Add(1)
-
-		go func() {
-			defer wg.Done()
-
-			<-start
-			for j := 0; j < 10; j++ {
-				// Before we had gitaly-ruby rate limiting, this could result in
-				// ResourceExhausted errors. This test is meant to guard against that
-				// problem coming back.
-				require.NoError(t, makeRequest(s))
-			}
-		}()
-	}
-
-	close(start) // increase chances that goroutines all run at once
-
-	wg.Wait()
-}
-
 func waitPing(s *Server) error {
 	var err error
 	for start := time.Now(); time.Since(start) < ConnectTimeout; time.Sleep(100 * time.Millisecond) {
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 0c07e9875..8b9f159dc 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -14,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/middleware/limithandler"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer"
 	"gitlab.com/gitlab-org/gitaly/internal/supervisor"
 	"gitlab.com/gitlab-org/gitaly/internal/version"
@@ -70,7 +69,6 @@ type Server struct {
 	workers      []*worker
 	clientConnMu sync.Mutex
 	clientConn   *grpc.ClientConn
-	limiter      *limithandler.ConcurrencyLimiter
 }
 
 // Stop shuts down the gitaly-ruby helper process and cleans up resources.
@@ -103,19 +101,6 @@ func Start() (*Server, error) {
 	}
 
 	cfg := config.Config
-
-	// We need a wide margin between the concurrency limit enforced on the
-	// client side and the thread pool size in the gitaly-ruby grpc server.
-	// If the client side limit is too close to the server thread pool size,
-	// or if the client does not limit at all, we can get ResourceExhausted
-	// errors from gitaly-ruby when it runs out of threads in its pool.
-	//
-	// Our choice of 2 * cfg.Ruby.Concurrency is probably not the optimal
-	// formula but it is good enough. Ruby threads are not that expensive,
-	// and it is pointless to set up large thread pools (e.g. 100 threads) in
-	// a single Ruby process anyway because of its Global Interpreter Lock.
-	rubyThreadPoolSize := 2 * cfg.Ruby.Concurrency
-
 	env := append(
 		os.Environ(),
 		"GITALY_RUBY_GIT_BIN_PATH="+command.GitPath(),
@@ -124,7 +109,6 @@ func Start() (*Server, error) {
 		"GITALY_RUBY_GITLAB_SHELL_PATH="+cfg.GitlabShell.Dir,
 		"GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir,
 		"GITALY_VERSION="+version.GetVersion(),
-		fmt.Sprintf("GITALY_RUBY_THREAD_POOL_SIZE=%d", rubyThreadPoolSize),
 	)
 	if dsn := cfg.Logging.RubySentryDSN; dsn != "" {
 		env = append(env, "SENTRY_DSN="+dsn)
@@ -132,9 +116,7 @@ func Start() (*Server, error) {
 
 	gitalyRuby := path.Join(cfg.Ruby.Dir, "bin/gitaly-ruby")
 
-	s := &Server{
-		limiter: limithandler.NewLimiter(cfg.Ruby.Concurrency, limithandler.NewPromMonitor("gitaly-ruby", "")),
-	}
+	s := &Server{}
 	for i := 0; i < cfg.Ruby.NumWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
 		socketPath := socketPath(i)
@@ -230,19 +212,6 @@ func (s *Server) BlobServiceClient(ctx context.Context) (pb.BlobServiceClient, e
 }
 
 func (s *Server) getConnection(ctx context.Context) (*grpc.ClientConn, error) {
-	ourTurn := make(chan struct{})
-	go s.limiter.Limit(ctx, "gitaly-ruby", func() (interface{}, error) {
-		close(ourTurn)
-		<-ctx.Done()
-		return nil, nil
-	})
-
-	select {
-	case <-ctx.Done():
-		return nil, ctx.Err()
-	case <-ourTurn:
-	}
-
 	s.clientConnMu.Lock()
 	conn := s.clientConn
 	s.clientConnMu.Unlock()
diff --git a/ruby/bin/gitaly-ruby b/ruby/bin/gitaly-ruby
index ce16e8ad4..87acda775 100755
--- a/ruby/bin/gitaly-ruby
+++ b/ruby/bin/gitaly-ruby
@@ -24,11 +24,7 @@ def main
   FileUtils.mkdir_p(socket_dir)
   File.chmod(0700, socket_dir)
 
-  pool_size = ENV['GITALY_RUBY_THREAD_POOL_SIZE'].to_i
-  pool_size = 30 if pool_size <= 0
-
   s = GRPC::RpcServer.new(
-    pool_size: pool_size,
     poll_period: SHUTDOWN_TIMEOUT,
     interceptors: [
       GitalyServer::SentryInterceptor.new,