Additional context deadline handling in makeRequestWithAuthTypeAndHeadersComplete

[amoiseiev]

Currently, the request retry / backoff logic is being applied to requests that reached context timeout. While it still works as we account for context deadline, it may be better to exit immediately. It also indirectly helps to safeguard from regressions like the ones introduced in v0.48.0 (fixed by #1072) as the response body is nil when go http client exits on context deadline (less processing/potentially less bugs).

Now, the error will just say that we reached the deadline for that specific request and doesn't imply we applied backoff logic or retried the request. It doesn't take away from the ability to surface more specific errors.

Also updating TestContextTimeout to:

1. better readability
2. making use of an actual http endpoint and increasing timeout values to make sure we get to api.request function and not timing out on rate limiter
3. ability to run the test individually

Has your change been tested?

Ran a request with short context timeout to reproduce the issue, confirmed it the expected result on timeout is long enough for a request to finish

	ctx, cncl := context.WithTimeout(context.Background(), time.Millisecond*time.Duration(50))
	defer cncl()

	// Getting existing A records from the zone
	_, err = api.DNSRecords(ctx, "<zone id>", cloudflare.DNSRecord{
		Type: "A",
		Name: "domain",
	})

Screenshots (if appropriate):

Types of changes

What sort of change does your code introduce/modify?

• [X ] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [X ] My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• [ X] All new and existing tests passed.
• This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

[github-actions]

changelog detected ✅

[jacobbednarz]

thanks for digging into this one. let's add this in a test with the assertion that we get the correct exception. once we have that, we're good to go.

[codecov-commenter]

Codecov Report

Merging #1080 (ea0951f) into master (e39278e) will increase coverage by 0.12%.
The diff coverage is 68.29%.

@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
+ Coverage   49.94%   50.07%   +0.12%     
==========================================
  Files         115      117       +2     
  Lines       10991    11090      +99     
==========================================
+ Hits         5490     5553      +63     
- Misses       4338     4362      +24     
- Partials     1163     1175      +12     

Impacted Files	Coverage Δ
pages_project.go	50.81% <ø> (ø)
stream.go	64.91% <ø> (ø)
teams_accounts.go	53.84% <ø> (ø)
teams_devices.go	52.63% <ø> (ø)
access_service_tokens.go	66.29% <53.84%> (-2.13%)	⬇️
api_shield.go	53.84% <53.84%> (ø)
url_normalization_settings.go	53.84% <53.84%> (ø)
workers.go	69.38% <72.72%> (-0.04%)	⬇️
cloudflare.go	68.61% <86.95%> (+1.41%)	⬆️
auditlogs.go	53.96% <100.00%> (+2.30%)	⬆️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[amoiseiev]

thanks for digging into this one. let's add this in a test with the assertion that we get the correct exception. once we have that, we're good to go.

So, we actually have a test called "TestContextTimeout" that is checking for error type, the reason it didn't catch v0.48 regression is because when we set context timeout to "0", the deadline gets reached on https://github.com/cloudflare/cloudflare-go/pull/1080/files#diff-432b285f81cb984bfd1ce00fa760be757c96784135a1b9ecd49d3b6c9a26522bR250 , even before we make a request.

The error that's currently being returned (before this change) is also of the right type, it's just extra wrapped in "backoff" error. Once the change is merged, it won't be wrapped by the "backoff" error but with retain context.DeadlineExceeded var type. I'm not sure it's worth looking for an unwrapped error in the test but happy to make the change if you have strong opinion on it.

I tweaked the test to spin up an actual endpoint to timeout on and increased the timeout value so we can get to api.request block, which makes it more realistic practical scenario. As a bonus, it's easier to understand what the test is doing and which URL it's calling. It also makes it run individually just fine now.

[jacobbednarz]

i think that updated test is great, thank you.

[github-actions]

This functionality has been released in v0.50.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!