Use http read total timeout by default (logging errors rather than raising)
What does this MR do?
Related to https://gitlab.com/gitlab-org/gitlab/-/issues/334930 (this is a confidential issue but it was decided that we don't have to follow the security workflow)
In https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1379 (internal only) we introduced a total read timeout for HTTP requests. This changed a fundamental part of the system, but we couldn't put it behind a feature flag and roll it out slowly while making sure it doesn't break anything. Instead, we added an opt-in option and used the new timeout only where it was necessary. Now it looks like the timeout is stable, so we can make it the default behavior.
This MR changes:
- all requests default to using the read total timeout, but only the existing calls that pass
use_read_total_timeout
will raise an exception (current behaviour) all other calls will instead log the error. This allows us to monitor impacts of making the read total timeout the default. - adds
skip_read_total_timeout
opt-out option. Skipping the total timeout is needed in some cases where a request is expected to be streamed, like downloading a file. -
response.to_s
toresponse.body
in several places becauseto_s
is not guaranteed to returnbody
as a string (see httparty/response.rb#L88-L94)
Does this MR meet the acceptance criteria?
Conformity
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Edited by Luke Duncalfe