A callback to go with SleepUntilPrimaryRateLimitResetWhenRateLimited?

[dee-kryvenko]

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]

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.

[dee-kryvenko]

@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

go-github/github/github.go

Line 894 in 020ef00

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

?

[gmlewis]

Sorry I misunderstood. Reopening.

[gmlewis]

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

[erezrokah]

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]

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]

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