Merge branch '42572-release-controller' into 'master'

Try not to hold env and release the controller after the request.

See merge request gitlab-org/gitlab-ce!16847

Gemfile.lock

@@ -601,7 +601,7 @@ GEM
       atomic (>= 1.0.0)
       mysql2
       peek
-    peek-performance_bar (1.3.0)
+    peek-performance_bar (1.3.1)
       peek (>= 0.1.0)
     peek-pg (1.3.0)
       concurrent-ruby

config.ru

@@ -23,5 +23,6 @@ warmup do |app|
 end
 
 map ENV['RAILS_RELATIVE_URL_ROOT'] || "/" do
+  use Gitlab::Middleware::ReleaseEnv
   run Gitlab::Application
 end

lib/gitlab/middleware/read_only.rb

 module Gitlab
   module Middleware
     class ReadOnly
-      DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze
-      APPLICATION_JSON = 'application/json'.freeze
       API_VERSIONS = (3..4)
 
+      def self.internal_routes
+        @internal_routes ||=
+          API_VERSIONS.map { |version| "api/v#{version}/internal" }
+      end
+
       def initialize(app)
         @app = app
-        @whitelisted = internal_routes
       end
 
       def call(env)
-        @env = env
-        @route_hash = nil
-
-        if disallowed_request? && Gitlab::Database.read_only?
-          Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation')
-          error_message = 'You cannot do writing operations on a read-only GitLab instance'
-
-          if json_request?
-            return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]]
-          else
-            rack_flash.alert = error_message
-            rack_session['flash'] = rack_flash.to_session_value
-
-            return [301, { 'Location' => last_visited_url }, []]
-          end
-        end
-
-        @app.call(env)
-      end
-
-      private
-
-      def internal_routes
-        API_VERSIONS.flat_map { |version| "api/v#{version}/internal" }
-      end
-
-      def disallowed_request?
-        DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && !whitelisted_routes
-      end
-
-      def json_request?
-        request.media_type == APPLICATION_JSON
-      end
-
-      def rack_flash
-        @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session)
-      end
-
-      def rack_session
-        @env['rack.session']
-      end
-
-      def request
-        @env['rack.request'] ||= Rack::Request.new(@env)
-      end
-
-      def last_visited_url
-        @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url
-      end
-
-      def route_hash
-        @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {}
-      end
-
-      def whitelisted_routes
-        grack_route || @whitelisted.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route
-      end
-
-      def sidekiq_route
-        request.path.start_with?('/admin/sidekiq')
-      end
-
-      def grack_route
-        # Calling route_hash may be expensive. Only do it if we think there's a possible match
-        return false unless request.path.end_with?('.git/git-upload-pack')
-
-        route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack'
-      end
-
-      def lfs_route
-        # Calling route_hash may be expensive. Only do it if we think there's a possible match
-        return false unless request.path.end_with?('/info/lfs/objects/batch')
-
-        route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch'
+        ReadOnly::Controller.new(@app, env).call
       end
     end
   end

lib/gitlab/middleware/read_only/controller.rb

+module Gitlab
+  module Middleware
+    class ReadOnly
+      class Controller
+        DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze
+        APPLICATION_JSON = 'application/json'.freeze
+        ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'.freeze
+
+        def initialize(app, env)
+          @app = app
+          @env = env
+        end
+
+        def call
+          if disallowed_request? && Gitlab::Database.read_only?
+            Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation')
+
+            if json_request?
+              return [403, { 'Content-Type' => APPLICATION_JSON }, [{ 'message' => ERROR_MESSAGE }.to_json]]
+            else
+              rack_flash.alert = ERROR_MESSAGE
+              rack_session['flash'] = rack_flash.to_session_value
+
+              return [301, { 'Location' => last_visited_url }, []]
+            end
+          end
+
+          @app.call(@env)
+        end
+
+        private
+
+        def disallowed_request?
+          DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) &&
+            !whitelisted_routes
+        end
+
+        def json_request?
+          request.media_type == APPLICATION_JSON
+        end
+
+        def rack_flash
+          @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session)
+        end
+
+        def rack_session
+          @env['rack.session']
+        end
+
+        def request
+          @env['rack.request'] ||= Rack::Request.new(@env)
+        end
+
+        def last_visited_url
+          @env['HTTP_REFERER'] || rack_session['user_return_to'] || Gitlab::Routing.url_helpers.root_url
+        end
+
+        def route_hash
+          @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {}
+        end
+
+        def whitelisted_routes
+          grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route
+        end
+
+        def sidekiq_route
+          request.path.start_with?('/admin/sidekiq')
+        end
+
+        def grack_route
+          # Calling route_hash may be expensive. Only do it if we think there's a possible match
+          return false unless request.path.end_with?('.git/git-upload-pack')
+
+          route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack'
+        end
+
+        def lfs_route
+          # Calling route_hash may be expensive. Only do it if we think there's a possible match
+          return false unless request.path.end_with?('/info/lfs/objects/batch')
+
+          route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch'
+        end
+      end
+    end
+  end
+end

lib/gitlab/middleware/release_env.rb

+module Gitlab
+  module Middleware
+    # Some of middleware would hold env for no good reason even after the
+    # request had already been processed, and we could not garbage collect
+    # them due to this. Put this middleware as the first middleware so that
+    # it would clear the env after the request is done, allowing GC gets a
+    # chance to release memory for the last request.
+    ReleaseEnv = Struct.new(:app) do
+      def call(env)
+        app.call(env).tap { env.clear }
+      end
+    end
+  end
+end

spec/lib/gitlab/middleware/read_only_spec.rb

@@ -11,15 +11,17 @@ describe Gitlab::Middleware::ReadOnly do
 
   RSpec::Matchers.define :disallow_request do
     match do |middleware|
-      flash = middleware.send(:rack_flash)
-      flash['alert'] && flash['alert'].include?('You cannot do writing operations')
+      alert = middleware.env['rack.session'].to_hash
+        .dig('flash', 'flashes', 'alert')
+
+      alert&.include?('You cannot perform write operations')
     end
   end
 
   RSpec::Matchers.define :disallow_request_in_json do
     match do |response|
       json_response = JSON.parse(response.body)
-      response.body.include?('You cannot do writing operations') && json_response.key?('message')
+      response.body.include?('You cannot perform write operations') && json_response.key?('message')
     end
   end
 
@@ -34,10 +36,25 @@ describe Gitlab::Middleware::ReadOnly do
     rack.to_app
   end
 
-  subject { described_class.new(fake_app) }
+  let(:observe_env) do
+    Module.new do
+      attr_reader :env
+
+      def call(env)
+        @env = env
+        super
+      end
+    end
+  end
 
   let(:request) { Rack::MockRequest.new(rack_stack) }
 
+  subject do
+    described_class.new(fake_app).tap do |app|
+      app.extend(observe_env)
+    end
+  end
+
   context 'normal requests to a read-only Gitlab instance' do
     let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
 

spec/lib/gitlab/middleware/release_env_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Middleware::ReleaseEnv do
+  let(:inner_app) { double(:app, call: 'yay') }
+  let(:app) { described_class.new(inner_app) }
+  let(:env) { { 'action_controller.instance' => 'something' } }
+
+  describe '#call' do
+    it 'calls the app and clears the env' do
+      result = app.call(env)
+
+      expect(result).to eq('yay')
+      expect(env).to be_empty
+    end
+  end
+end