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

A callback to go with SleepUntilPrimaryRateLimitResetWhenRateLimited? #3220

Open
dee-kryvenko opened this issue Jul 26, 2024 · 7 comments
Open

Comments

@dee-kryvenko
Copy link

A library this repository suggests for secondary rate limits https://github.com/gofri/go-github-ratelimit
already has a similar concept WithLimitDetectedCallback(callback). There is no easy way right now to handle sleeps i.e. log a message to the terminal or something...

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 26, 2024

Sleeps should be handled in your client code that uses this library.

We have an entire section in our README.md devoted to the topic:
https://github.com/google/go-github/tree/master?tab=readme-ov-file#rate-limiting

I hope that helps.

@gmlewis gmlewis closed this as completed Jul 26, 2024
@dee-kryvenko
Copy link
Author

@gmlewis I am sorry, this response is confusing. This repository already have a sleep logic, and it is explained in the README.md right below the place you linked:

repos, _, err := client.Repositories.List(context.WithValue(ctx, github.SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "", nil)

What I am talking about, is a callback next to the sleep, because right now the information that the sleep did occur is not exposed outside so my program would just freeze without an opportunity to me notify a user or log a message.

I am not sharing the thought that the client code should handle this. I can as well just use http client instead of this library. And ASM instead of Go. How deep should we go, really? How is the amount of code the client needs to do to check for the rate limit and semaphore in a multi threaded environment can be justified comparatively to three lines it takes to invoke a callback right around here

if ok && req.Context().Value(SleepUntilPrimaryRateLimitResetWhenRateLimited) != nil {
?

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 26, 2024

Sorry I misunderstood. Reopening.

@gmlewis gmlewis reopened this Jul 26, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Jul 26, 2024

@erezrokah - can you please comment on this issue since you added this feature in #3117 ?

@erezrokah
Copy link
Contributor

Hi @dee-kryvenko and @gmlewis the feature request makes sense to me, it's even listed in the possible future improvements in the original PR #3117.

If anyone has a suggestion on how to configure the callback, happy to comment/review it on it

@dee-kryvenko
Copy link
Author

dee-kryvenko commented Jul 29, 2024

Well, if we were to keep the precedent that is already established, it sounds like we need a new context key with function as a value. I think function should have a simple no args no returns signature and upstream code can handle everything asynchronous via channels. So this needs to just check if the context is set, cast the value, and call it. Question is - do we want this to be at the level of checking existing context key, which I can see two places there, or do we want to put that directly in the sleep function?

@erezrokah
Copy link
Contributor

I would follow a similar behavior as in https://github.com/gofri/go-github-ratelimit?tab=readme-ov-file#client-options, where the callback accepts some information on the request that is throttled, the sleep time, etc. and is invoked just before the sleep happens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants