Skip to content

Use http read total timeout by default (logging errors rather than raising)

Andy Schoenen requested to merge 334930_use-http-total-timeout-by-default into master

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 to response.body in several places because to_s is not guaranteed to return body as a string (see httparty/response.rb#L88-L94)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Edited by Luke Duncalfe

Merge request reports