Adaptive request timeouts

[pietern]

Changes

The net/http package supports only a fixed timeout.

This is problematic when streaming a large request or response body.

The approach proposed in this PR is to disable the net/http timeouts and use our own context for cancellation instead. This context is canceled only if there is no activity for the duration of 1 timeout period. As the request or response body is read, the deadline is reinitialized to equal 1 timeout period from the current timestamp. This means real timeouts will still time out in the same amount of time, but streaming requests will not time out as long as there is activity.

If a request times out, it used to return the following error:

Get "/a?": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Now it returns the following error:

Get "/a?": request timed out after 10ms of inactivity

Note: the unit tests are time-based. Not great but I don't have a great alternative (that doesn't involve mocking time itself).

Tests

• make test passing
• make fmt applied
• relevant integration tests applied

[pietern]

I confirmed that with a very tight timeout (5 seconds), the CLI no longer times out when working with huge files and Unity Catalog Volumes. Ad hoc debug logging confirms that the deadline keeps getting extended while the request/response body is being read. Requests succeed regardless of the network bandwidth as long as there is activity.

[mgyucht]

Ok, dug through the guts of net/http and I believe the client uses a 32kb buffer when reading from the request body. So this would mean that if less than 32kb is written out over HTTPTimeout duration, the request could be broken. Given the default 30s timeout, the user needs to sustain more than 1066b/s. Sounds good to me.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.23810% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 18.54%. Comparing base (28be042) to head (1ee2336).
Report is 1 commits behind head on main.

Files	Patch %	Lines
httpclient/timeout_context.go	94.44%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #837      +/-   ##
==========================================
+ Coverage   18.33%   18.54%   +0.21%     
==========================================
  Files         118      119       +1     
  Lines       22166    22227      +61     
==========================================
+ Hits         4064     4122      +58     
- Misses      17851    17853       +2     
- Partials      251      252       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pietern]

Kicking off an integration test run now.

[pietern]

@mgyucht I needed to check for http.NoBody as well. The guts of net/http use this to signal an empty body if it is not nil... We clearly don't care about extending timeouts if there is no body, so safe to filter here.

[mgyucht]

LGTM. Thanks for the clear comments!

[pietern]

Integration tests pass.