search

DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.17k stars 39 forks source link

util: Use GitHub rate_limit endpoint with greater is_online timeout #304

Closed sonic2kk closed 8 months ago

sonic2kk commented 8 months ago

May help with #303.

Changing default host

When skimming the code and doing tests on my own PC for #303, I noticed requests was coming back with a <Response [404]>. Turns out the repos endpoint doesn't exist, or no longer exists. This doesn't break is_online, as we don't care about the status code, we just care that we can reach api.github.com. But I figured it might be better practice to hit a valid endpoint.

One user reported that ProtonUp-Qt will report that they are offline for them no matter what. Since is_online simply uses requests to ping an endpoint with a given request, I have two theories:

  1. They are rate-limited, but I don't think requests would throw a ConnectionError for a rate limt, and it's even less likely that they would throw a Timeout (these are the two exceptions we check for).
  2. The request is timing out, and I think this may be the case.

As noted, my money right now is on a timeout. Currently, we set the timeout to 3, and this has been fine in all of my tests for about 10 months since this feature was added. Even on my Steam Deck this has worked fine, and for this test I just checked again and it still works fine.

Changing timeout

My guess is then, that is_online is not forgiving enough for networks with higher latency. On devices like the Steam Deck or even some laptops, when Steam is opened, it could be using a lot of bandwidth (i.e. resuming downloading/updating games) automatically on top of the connection not being very strong. Therefore, the connection times out because it takes a little bit longer than 3 seconds. In my tests, the request on my network on my PC takes less than 0.2 seconds, 0.15s on average. And I can get is_online to return False (and ProtonUp-Qt displays "(Offline)") if I set the timeout to be 0.1 or less. I can get it to intermittently fail if I set it to 0.15.

I don't think we have any way to know if the connection is actually timing out, but that is just my assumption.

I have currently set the timeout to 5, and it doesn't add any noticeable startup delay, but that is almost certainly because the request completes very quickly. If we set the timeout to, say, 30, and someone on a very slow network did take close to that, ProtonUp-Qt would likely stall waiting for the request to complete and it means it could take up to 30 seconds.

I guess we have a balancing act here, I don't know if it's worth the effort to make this check threaded, and it probably doesn't make much sense to make it threaded anyway since we want to show this as early as possible. But maybe having a more lenient timeout could resolve the problems. 5 might be a bit low, so I'd be curious to hear what your thoughts are :smile:

DavidoTek commented 8 months ago

May help with #303.

Yeah, that kinda sounds like a timeout issue. We need to wait and see what the reporting person responds to your questions.

Turns out the repos endpoint doesn't exist, or no longer exists

I'm not sure whether the /repos endpoint actually existed at some point, but it seems reasonable to change it to some existing endpoint as you never know how the API reacts to an unknown request (e.g., calling an unknown endpoint could in theory add a delay).

I actually wasn't aware of GitHub's /rate_limit endpoint. That could be useful for other stuff :thinking:

I have currently set the timeout to 5, and it doesn't add any noticeable startup delay

I guess that is fine. There are only two places where is_online is utilized:

  1. When updating the status message. This is done in a separate thread, so it shouldn't slow down anything.
  2. When trying to fetch releases. We need to wait for a connection here anyway. 5 seconds is probably a reasonable time to wait for a timeout. It will be faster in most cases anyway.

I will wait for a response in #303 to see if there is anything else to consider. Still, I think this PR makes sense and will be merged later.

Thanks!

sonic2kk commented 8 months ago

I actually wasn't aware of GitHub's /rate_limit endpoint. That could be useful for other stuff

I wasn't either until I went to https://api.github.com/ to see if that alone would be viable alternative to /repos. I wanted to see if it could be accessed standalone, and I happened to spot /rate_limit.

It could be useful to parse info from here, and just to note, I believe GitLab does things slightly differently.

As far as I can tell, GitHub doesn't give back rate limit info in the headers because it has a general overarching rate limit, but GitLab has a different system where each type of API call has a different rate limit (you could be rate-limited for fetching account info but still get release info, for example). And from what I can see, GitLab gives this information back in the response headers. Just something to keep in mind if we ever change the ratelimit check and want to account for GitLab and GitHub :-)