Merge branch '334930_use-http-total-timeout-by-default' into 'master'

Use http read total timeout by default

See merge request gitlab-org/gitlab!66487

app/services/dependency_proxy/pull_manifest_service.rb

@@ -17,7 +17,7 @@ module DependencyProxy
         file = Tempfile.new
 
         begin
-          file.write(response)
+          file.write(response.body)
           file.flush
 
           yield(success(file: file, digest: response.headers['docker-content-digest'], content_type: response.headers['content-type']))

ee/app/models/app_sec/fuzzing/api/ci_configuration.rb

@@ -39,7 +39,7 @@ module AppSec
           response = Gitlab::HTTP.try_get(PROFILES_DEFINITION_FILE)
 
           if response&.success?
-            content = Gitlab::Config::Loader::Yaml.new(response.to_s).load!
+            content = Gitlab::Config::Loader::Yaml.new(response.body).load!
 
             content.fetch(:Profiles, [])
           else

lib/gitlab/auth/otp/strategies/forti_token_cloud.rb

@@ -35,7 +35,7 @@ module Gitlab
           end
 
           def access_token
-            Gitlab::Json.parse(access_token_create_response)['access_token']
+            Gitlab::Json.parse(access_token_create_response.body)['access_token']
           end
 
           def verify_otp(otp_code)

lib/gitlab/ci/config/external/file/remote.rb

@@ -45,7 +45,7 @@ module Gitlab
                 errors.push("Remote file `#{location}` could not be fetched because of HTTP code `#{response.code}` error!")
               end
 
-              response.to_s if errors.none?
+              response.body if errors.none?
             end
           end
         end

lib/gitlab/http.rb

@@ -43,16 +43,29 @@ module Gitlab
           options
         end
 
-      unless options.has_key?(:use_read_total_timeout)
+      options[:skip_read_total_timeout] = true if options[:skip_read_total_timeout].nil? && options[:stream_body]
+
+      if options[:skip_read_total_timeout]
         return httparty_perform_request(http_method, path, options_with_timeouts, &block)
       end
 
       start_time = Gitlab::Metrics::System.monotonic_time
       read_total_timeout = options.fetch(:timeout, DEFAULT_READ_TOTAL_TIMEOUT)
+      tracked_timeout_error = false
 
       httparty_perform_request(http_method, path, options_with_timeouts) do |fragment|
         elapsed = Gitlab::Metrics::System.monotonic_time - start_time
-        raise ReadTotalTimeout, "Request timed out after #{elapsed} seconds" if elapsed > read_total_timeout
+
+        if elapsed > read_total_timeout
+          error = ReadTotalTimeout.new("Request timed out after #{elapsed} seconds")
+
+          raise error if options[:use_read_total_timeout]
+
+          unless tracked_timeout_error
+            Gitlab::ErrorTracking.track_exception(error)
+            tracked_timeout_error = true
+          end
+        end
 
         block.call fragment if block
       end

lib/gitlab/jira/http_client.rb

@@ -40,6 +40,14 @@ module Gitlab
         @authenticated = result.response.is_a?(Net::HTTPOK)
         store_cookies(result) if options[:use_cookies]
 
+        # This is needed to make response.to_s work. HTTParty::Response internal uses a Net::HTTPResponse as @response.
+        # When a block is used, Net::HTTPResponse#body will be a Net::ReadAdapter instead of a String.
+        # In this case HTTParty::Response.to_s will default to inspecting the Net::HTTPResponse class instead
+        # of returning the content of body.
+        # See https://github.com/jnunemaker/httparty/blob/v0.18.1/lib/httparty/response.rb#L86-L92
+        # See https://github.com/ruby/net-http/blob/v0.1.1/lib/net/http/response.rb#L346-L350
+        result.response.body = result.body
+
         result
       end
 

spec/lib/gitlab/http_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe Gitlab::HTTP do
 
       WebMock.stub_request(:post, /.*/).to_return do |request|
         sleep 0.002.seconds
-        { body: 'I\m slow', status: 200 }
+        { body: 'I\'m slow', status: 200 }
       end
     end
 
@@ -41,25 +41,67 @@ RSpec.describe Gitlab::HTTP do
 
     subject(:request_slow_responder) { described_class.post('http://example.org', **options) }
 
-    specify do
-      expect { request_slow_responder }.not_to raise_error
+    shared_examples 'tracks the timeout but does not raise an error' do
+      specify :aggregate_failures do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+          an_instance_of(Gitlab::HTTP::ReadTotalTimeout)
+        ).once
+
+        expect { request_slow_responder }.not_to raise_error
+      end
+
+      it 'still calls the block' do
+        expect { |b| described_class.post('http://example.org', **options, &b) }.to yield_with_args
+      end
     end
 
-    context 'with use_read_total_timeout option' do
+    shared_examples 'does not track or raise timeout error' do
+      specify :aggregate_failures do
+        expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
+
+        expect { request_slow_responder }.not_to raise_error
+      end
+    end
+
+    it_behaves_like 'tracks the timeout but does not raise an error'
+
+    context 'and use_read_total_timeout option is truthy' do
       let(:options) { { use_read_total_timeout: true } }
 
-      it 'raises a timeout error' do
+      it 'raises an error' do
         expect { request_slow_responder }.to raise_error(Gitlab::HTTP::ReadTotalTimeout, /Request timed out after ?([0-9]*[.])?[0-9]+ seconds/)
       end
+    end
 
-      context 'and timeout option' do
-        let(:options) { { use_read_total_timeout: true, timeout: 10.seconds } }
+    context 'and timeout option is greater than DEFAULT_READ_TOTAL_TIMEOUT' do
+      let(:options) { { timeout: 10.seconds } }
 
-        it 'overrides the default timeout when timeout option is present' do
-          expect { request_slow_responder }.not_to raise_error
-        end
+      it_behaves_like 'does not track or raise timeout error'
+    end
+
+    context 'and stream_body option is truthy' do
+      let(:options) { { stream_body: true } }
+
+      it_behaves_like 'does not track or raise timeout error'
+
+      context 'but skip_read_total_timeout option is falsey' do
+        let(:options) { { stream_body: true, skip_read_total_timeout: false } }
+
+        it_behaves_like 'tracks the timeout but does not raise an error'
       end
     end
+
+    context 'and skip_read_total_timeout option is truthy' do
+      let(:options) { { skip_read_total_timeout: true } }
+
+      it_behaves_like 'does not track or raise timeout error'
+    end
+
+    context 'and skip_read_total_timeout option is falsely' do
+      let(:options) { { skip_read_total_timeout: false } }
+
+      it_behaves_like 'tracks the timeout but does not raise an error'
+    end
   end
 
   it 'calls a block' do

spec/services/dependency_proxy/download_blob_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe DependencyProxy::DownloadBlobService do
   let(:token) { Digest::SHA256.hexdigest('123') }
   let(:blob_sha) { Digest::SHA256.hexdigest('ruby:2.7.0') }
 
-  subject { described_class.new(image, blob_sha, token).execute }
+  subject(:download_blob) { described_class.new(image, blob_sha, token).execute }
 
   context 'remote request is successful' do
     before do
@@ -18,6 +18,21 @@ RSpec.describe DependencyProxy::DownloadBlobService do
     it { expect(subject[:status]).to eq(:success) }
     it { expect(subject[:file]).to be_a(Tempfile) }
     it { expect(subject[:file].size).to eq(6) }
+
+    it 'streams the download' do
+      expected_options = { headers: anything, stream_body: true }
+
+      expect(Gitlab::HTTP).to receive(:perform_request).with(Net::HTTP::Get, anything, expected_options)
+
+      download_blob
+    end
+
+    it 'skips read_total_timeout', :aggregate_failures do
+      stub_const('GitLab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT', 0)
+
+      expect(Gitlab::Metrics::System).not_to receive(:monotonic_time)
+      expect(download_blob).to include(status: :success)
+    end
   end
 
   context 'remote request is not found' do

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -90,6 +90,21 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do
 
         expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
       end
+
+      it 'streams the download' do
+        expected_options = { headers: anything, stream_body: true }
+
+        expect(Gitlab::HTTP).to receive(:perform_request).with(Net::HTTP::Get, anything, expected_options)
+
+        subject.execute
+      end
+
+      it 'skips read_total_timeout', :aggregate_failures do
+        stub_const('GitLab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT', 0)
+
+        expect(Gitlab::Metrics::System).not_to receive(:monotonic_time)
+        expect(subject.execute).to include(status: :success)
+      end
     end
 
     context 'when file download fails' do