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

Add check in main.py for hitting the ratelimit #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add check in main.py for hitting the ratelimit #6

wants to merge 2 commits into from

Conversation

Titaniumtown
Copy link

Temp fix for #4 in main.py
(#5 does the same thing but for artifact_lib.py)

main.py Outdated Show resolved Hide resolved
remaining = json['resources']['core']['remaining']
return remaining

if test_rate() == 0:
Copy link
Owner

@JayFoxRox JayFoxRox Aug 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit aggressive; can't we just check if we got an error (on the request), and then report it?
Why would we have to explicitly check for errors independently of the actual request?

I feel this adds a https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just felt it was cleaner. Idk though.

@JayFoxRox
Copy link
Owner

I haven't used this project in a while, because of #4 , so I didn't test this change.
However, how could this possibly work? I don't see how it fixes #4, because this PR doesn't add any authentication at all. Rate-Limiting would be a separate issue.

@Titaniumtown
Copy link
Author

Titaniumtown commented Aug 24, 2020

Well, it would fix the issue of throwing errors when you hit that rate limit, if you were authenticated, afaik, there is a much higher cap on your rate limit.

@JayFoxRox
Copy link
Owner

Well, it would fix the issue of throwing errors when you hit that rate limit

I don't think it fixes anything; it's merely a temporary workaround.

But even then it makes it harder to catch these errors because it's hard to catch the hard exit().
Instead, the existing code will likely throw some kind of exception which can be caught (which should be extended to throw cleaner exceptions, for all potential errors, rather than just the rate-limit issue).

if you were authenticated, afaik, there is a much higher cap on your rate limit.

Yes, but if you aren't authenticated, last I checked, for this API (or the resulting URLs), the limit was... (in practice) 0 requests.
I say "in practice" because it's not the rate-limiting that gets you, but simply permission issues in the GitHub Actions design.

Until we add authentication, there is no way this script could work.

So this is not a rate-limiting issue, but an authentication issue.
Displaying this as a rate-limit error would be misleading.

Once we add authentication we might hit rate-limits, but then I still think that it should be handled differently for the problems I described in my review comments (TOCTOU) and elsewhere.

The check is too specific for one type of problem, so we'd be playing cat-and-mouse when other kind of errors arise. We should be able to handle errors when they arise, rather than speculatively checking if they might happen.
The number of server interactions should be minimized for performance; there's no need for knowing our rate-limit in advance, until it's too late.

@Titaniumtown
Copy link
Author

Oh, I misunderstood #4 lol. I get what you mean now.

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

Successfully merging this pull request may close these issues.

2 participants