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

Contributors list stalling due to rate limiting (need helpful error message) #284

Open
jywarren opened this issue Oct 19, 2019 · 8 comments · May be fixed by #296
Open

Contributors list stalling due to rate limiting (need helpful error message) #284

jywarren opened this issue Oct 19, 2019 · 8 comments · May be fixed by #296

Comments

@jywarren
Copy link
Member

Hi, I'm not sure if this is related to #112 but probably not -- I noticed that after seeing it work for a while, i seem to be in a stuck state where the contributors list is not loading:

image

Is it possible I'm getting rate limited or something? And if so, should I be getting a specific error saying so? @Rishabh570 i know you're a bit busy right now but wondering if you could ID the failure point so we can work on this a bit. Thanks!

@jywarren
Copy link
Member Author

Here's one error at least I'm seeing:

Failed to load resource: the server responded with a status of 403 (Forbidden)

@jywarren
Copy link
Member Author

So, just noting that if we do hit rate limits, maybe we could catch and handle that error and display a message saying like API rate limit encountered, please wait ____ minutes to refresh #53

@jywarren jywarren changed the title Contributors list stalling sometimes Contributors list stalling due to rate limiting (need helpful error message) Oct 19, 2019
@jywarren
Copy link
Member Author

jywarren commented Oct 19, 2019

Aha, i see @Rishabh570 found the error returned, it's a 403 so we have to recognize it:

Yep, we have to put catch blocks in places where network calls happening...but I observed that the error object doesn't contain API limit exceeded for ..., it only has a status of 403, so we can put a simple statement checking if the error.status == 403 and if its true, we can call some function from ui.js to show the appropriate message to the user...

For now at least it's probably enough to just detect the error code being 403. I updated the title accordingly!

@jywarren
Copy link
Member Author

I believe the request is generated here!

function getCommitsInRange(username, repo, options){
url = "https://api.github.com/repos/"+username+"/"+repo+"/commits?since=" + options["since"]["date"]+ "T" + options["since"]["time"] + "Z"+"&until="+options["until"]["date"]+"T"+ options["until"]["time"] + "Z";
result = httpGet(url);
return JSON.parse(result);
}
// send GET request
function httpGet(url){
var xmlHttp = new XMLHttpRequest();
xmlHttp.open( "GET", url, false ); // false for synchronous request
xmlHttp.send( null );
return xmlHttp.responseText;
}

@somenath1435
Copy link
Contributor

@jywarren @Rishabh570 can I work on this issue??

@Rishabh570
Copy link
Collaborator

@somenath1435 Yeah, sure. You can work on it if you'd like to contribute.

So, this file fetches contributors of a particular repository, the catch block throws the error which goes to fetchRepoContribsUtil.js (this might be the cause, I reckon it is not propagating the error further in the pipeline which is resulting in notification not being shown). This fetchRepoContribsUtil.js ideally should be throwing error to contribsUtil/main and which should eventually throw the error to community-toolbox.js which finally shows the error.

Feel free to ask if you need any help 👍

@somenath1435
Copy link
Contributor

@Rishabh570 How can I communicate with you? I can't find you in gitter

@jywarren
Copy link
Member Author

jywarren commented Oct 8, 2021

Another short term fix could be to add a message like

Fetching from GitHub API. This may stall if rate limit is exceeded; if so, try refreshing in X minutes.

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

Successfully merging a pull request may close this issue.

3 participants