Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MAINT]: Make library Timeout's actually useful #2956

Open
1 task done
caesay opened this issue Jul 26, 2024 · 2 comments
Open
1 task done

[MAINT]: Make library Timeout's actually useful #2956

caesay opened this issue Jul 26, 2024 · 2 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@caesay
Copy link

caesay commented Jul 26, 2024

Describe the need

At the moment the timeout support is confusing and inadequate. Take the following method for example:

private async Task UploadFileAsAsset(GitHubClient client, Release release, string filePath)
{
    using var stream = File.OpenRead(filePath);
    var data = new ReleaseAssetUpload(Path.GetFileName(filePath), "application/octet-stream", stream, TimeSpan.FromMinutes(60));
    await client.Repository.Release.UploadAsset(release, data, CancellationToken.None);
}

You might think that this request will timeout after 60 minutes. Actually, this request will timeout after 100 seconds which is default connection timeout used by HttpClient. The ReleaseAssetUpload.Timeout value you see being set above will create a CancellationToken which will cancel the request prematurely (see HttpClientAdapter.cs#L61) but it does not override the HttpClient timeout. And 100 seconds not enough time to upload a larger asset.

It is possible to override the HttpClient timeout with client.SetRequestTimeout(...), however this now applies to every request. If I SetRequestTimeout to 60 minutes, even a simple "list releases" or "create release" request will be allowed to run for 60 minutes before timing out! That is not appropriate.

To add insult to injury, if I set the HttpClient request timeout to something large, I can't even specify the cancellation-token style timeout on the short-lived requests so they will timeout in a reasonable timeframe. (because most Octokit requests do not expose the Timeout property)

My options currently are:

  • Ignore the ReleaseAssetUpload.Timeout (because it's useless), and constantly call SetRequestTimeout before each request with a reasonable time. (can't do this because apparently SetRequestTimeout can not be updated after the first request)
  • Set both the SetRequestTimeout and ReleaseAssetUpload.Timeout to comically large values, and create my own cancellation token's for every request with reasonable values. (can't do this, because most requests don't even allow you to supply a cancellation token)

However, in both of the above options it's clear that the built-in timeout logic in Octokit is not fit for purpose.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@caesay caesay added Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Jul 26, 2024
@caesay caesay changed the title [MAINT]: Clear up Timeout situation [MAINT]: Make library Timeout's actually useful Jul 26, 2024
@caesay
Copy link
Author

caesay commented Jul 30, 2024

Just writing to say that both of the workarounds I'd come up with did not work, so actually the only solution is to just set an insanely large timeout for every request.

@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Aug 2, 2024
@ElanHasson
Copy link

ugh, also no CancellationToken support?
:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Status: 🔥 Backlog
Development

No branches or pull requests

3 participants