Skip to content

Feature/rate limited requests#38

Merged
testpushpleaseignore merged 2 commits intotestpushpleaseignore:masterfrom
gerwaric:feature/rate-limited-requests
Aug 6, 2023
Merged

Feature/rate limited requests#38
testpushpleaseignore merged 2 commits intotestpushpleaseignore:masterfrom
gerwaric:feature/rate-limited-requests

Conversation

@gerwaric
Copy link

@gerwaric gerwaric commented Aug 6, 2023

The changes in this pull fixes acquisition so that it dynamically throttles API requests according the the rate limit policies parsed from HTTP reply headers.

There is a new RateLimit namespace implemented by "ratelimit.h" and "ratelimit.cpp" that provides static Init() and Submit() functions that are used by the ItemsMananagerWorker object. This new Submit() function replaces calls to network_manager_.get(). The Submit() accepts a QNetworkRequest and a callback function that takes a QNetworkReply* as it's only argument. The callback functions are guaranteed to be executed in the order they were submitted, even if the underlying network requests occur in a different order. This can happen when requests to different endpoints need to be throttle differently due to different policies being applied to each endpoint.

Rate limit policies and associated endpoints are defined in "ratelimits.json". The policy names and endpoints need to be defined at startup. The actual rate limits and state of those limits are dynamically parsed from HTTP responses. It would be possible to have endpoints and policies be recognized and setup dynamically. However, all the endpoints used in Acquisition are known at compile time, so there's no need to add runtime complexity to the RateLimit code.

There is also a default rate limit manager than handles non-rate-limited requests, such as those to the Path of Exile main page, or other domains. This is useful because it causes even non-API requests to come back in order alongside any API requests that have been made.

Rate limit violations can still occur, for example, if someone restarts Acquisition after encountering a rate limit violation, and the restriction period hasn't expired. The other way violations can sneak is if there are other API requests that Acquisition doesn't know about, e.g. someone using their web browser to manually make requests, or using another tool that makes similar API requests.

I have tested the debug builds in QT Creator 11.0.1 and Visual Studio 2019.

I apologies for any mind-boggling design choices in my implementation. I'm new to this.

tomholz added 2 commits August 6, 2023 08:08
Dynamically throttles API requests according the the rate limit policies in the HTTP reply headers.
@testpushpleaseignore
Copy link
Owner

Yeah these changes were exactly what I was hoping to do myself, had I had the time :).

I can accept these here right now, and release a new installer with these changes, full credit to you.

Thanks!

@testpushpleaseignore testpushpleaseignore merged commit 16bc57a into testpushpleaseignore:master Aug 6, 2023
@gerwaric
Copy link
Author

gerwaric commented Aug 7, 2023

Thanks. It definitely needs testing; I've only tried on my main account, which has just over 200 tabs in Standard. I think it took on the order of an hour or two to download everything. If this approach works there's probably some code that can be removed to simply ItemsManagerWorker later, too.

If you're not planning an announcement in /r/pathofexile, I'd be happy to make one as /u/gerwaric.

@gerwaric gerwaric deleted the feature/rate-limited-requests branch August 7, 2023 02:24
@aiolos01
Copy link

aiolos01 commented Aug 7, 2023

Unfortunately it doesn't work for me. I get thousands of errors in a couple of minutes with only 19 tabs downloaded. No other apps that get the stash are used, no browser windows etc.

It's actually worse than before. The previous version downloaded 100+ tabs before the hard limit stopped it permanently.

non stop multiple times per second:
"backend-character-request-limit" RATE LIMIT VIOLATION on request 5 of 113 seconds

@resrep2
Copy link

resrep2 commented Aug 7, 2023

I'm not playing right now so I can't test it but I did play earlier on crucible and the rate limits were very annoying.

Is it possible to have an option to start the program without updating any characters or stash tabs at all and just manually update what you need after its logged in? I think that would be a simple fix.

@aiolos01
Copy link

aiolos01 commented Aug 7, 2023

I'm not playing right now so I can't test it but I did play earlier on crucible and the rate limits were very annoying.

Is it possible to have an option to start the program without updating any characters or stash tabs at all and just manually update what you need after its logged in? I think that would be a simple fix.

This is exactly what I suggested a couple of months ago in issue #32. I assume implementing this should really be as easy as commenting out a single line of code.

@gerwaric
Copy link
Author

gerwaric commented Aug 7, 2023

@aiolos01 Some questions that might help debugging. Are you using your email, Steam, or the POESESSID from a logged-in browser session to authenticate? Which league are you requesting? What timezone and country are you in?

You mentioned "request 5". That's the first request that Acquisition sends that is rate-limited. It's the one that gets the character list for your account. Apart from a login-related bug, my first guess is that there's a problem in how I'm parsing dates from the headers, especially if you and I are in different time zones, or using different servers.

@testpushpleaseignore is there a way to get a build with the debug level set to trace to @aiolos01 ? I'd love to see exactly what is going on under the hood.

@gerwaric
Copy link
Author

gerwaric commented Aug 7, 2023

After some investigation, I see another change I can make. It looks like we can use HEAD requests to determine which policy applies to an endpoint without counting as a hit against that endpoint. This also tells you the state of that policy. If this works for all the endpoints Acquisition uses, we can almost certainly avoid rate limit violations on program startup, and it could also mean we don't need to hard-code the rate limit policies and endpoints in ratelimits.json.

(If I can get this working I'll create another pull request. I have never used GitHub before, so someone please let me know if there's a better way to handle this.)

@resrep2
Copy link

resrep2 commented Aug 7, 2023

I'm not playing right now so I can't test it but I did play earlier on crucible and the rate limits were very annoying.
Is it possible to have an option to start the program without updating any characters or stash tabs at all and just manually update what you need after its logged in? I think that would be a simple fix.

This is exactly what I suggested a couple of months ago in issue #32. I assume implementing this should really be as easy as commenting out a single line of code.

I don't know if its as simple as changing one line or not but it seems like an easier solution than trying to get around rate limits, plus if in the future they change the rates again the program would need to be updated again. Still, any fix is nice to have and I will try it out next league.

@came-here-to-learn
Copy link

came-here-to-learn commented Aug 7, 2023

This completely doesnt work. It hammers the servers with hundreds of requests every minute (POESSID). Opposite of what GGG wanted.

Why cant you just hardcode it to download 3 tabs, then wait 5 minutes. Then download another 3 tabs, wait another 5 minutes?
Or set X (how many tabs) and Y (per how many minutes) that users can select? With maybe an option to sleep Z minutes every J tabs (just to be tripple sure).

Id rather leave my computer turned on for 24h to index everything slowly, rather than hammer GGG with 1000 requests per minute what temp bans me for 2 weeks.

Something went very wrong that the app does not stop trying to download when it gets blocked by the server.

t1111

@gerwaric
Copy link
Author

gerwaric commented Aug 8, 2023

@testpushpleaseignore, this release needs to be pulled so Acquisition isn't hammering GGG's servers. That not only risks individual users, but may get Acquisition added to a block list.

Unfortunately this release works for me, so I can't replicate the problem, which means I can't fix it. I'm trying, but there's something going on with your two systems that's different from mine.

@came-here-to-learn if you're technically minded, I'd love to have your help debugging, since you're experiencing the problem.

@gerwaric
Copy link
Author

gerwaric commented Aug 8, 2023

I don't know if its as simple as changing one line or not but it seems like an easier solution than trying to get around rate limits, plus if in the future they change the rates again the program would need to be updated again. Still, any fix is nice to have and I will try it out next league.

Your idea of a very slow hard-coded rate limit is the simplest solution. Looking at the rate limits GGG is applying now, we could safely make 2 requests per minute indefinitely. I think the limit that we need to avoid is 100 requests in 1800 seconds to the endpoints controlled by the backend-item-request-limit policy. That limit technically allows us to send 1 request every 18 seconds, but it would be nice to only send every 30-60 seconds so there is room for GGG to tighten the rate limits without breaking acquisition again.

@gerwaric
Copy link
Author

gerwaric commented Aug 8, 2023

I've just made a pull request for the slower simpler approach with hard-coded limits, so hopefully @testpushpleaseignore can review and release it as a new version asap: #39

That way people can at least use Acquisition again until someone can help figure out what was going wrong with the dynamic approach.

@aiolos01
Copy link

aiolos01 commented Aug 8, 2023

Since you're working on this can you please also disable the refresh on application start? Or make it optional like refresh every X minutes.
It's really not necessary. We all have lots of remove only tabs that never change and it's pointless to constantly try to refresh them.

@resrep2
Copy link

resrep2 commented Aug 8, 2023

I don't know if its as simple as changing one line or not but it seems like an easier solution than trying to get around rate limits, plus if in the future they change the rates again the program would need to be updated again. Still, any fix is nice to have and I will try it out next league.

Your idea of a very slow hard-coded rate limit is the simplest solution. Looking at the rate limits GGG is applying now, we could safely make 2 requests per minute indefinitely. I think the limit that we need to avoid is 100 requests in 1800 seconds to the endpoints controlled by the backend-item-request-limit policy. That limit technically allows us to send 1 request every 18 seconds, but it would be nice to only send every 30-60 seconds so there is room for GGG to tighten the rate limits without breaking acquisition again.

The hard coded rate limit was @came-here-to-learn 's idea, I think a safe hard coded limit is definitely needed in case they further restrict requests in the future but also personally I still think the best way to do it is what @aiolos01 said, manual updating.

If you're an old player you probably have way too many stash tabs and characters that don't need updating when you only actively use 2 or 3 tabs and everything else is just old stuff, so really just a box you can tick that says "don't automatically update tabs on startup" and makes the program only retrieve the file with your tab and character list and not update the actual contents of the tabs and character inventories would fix the problem because it would eliminate hundred of requests that don't need to happen anyway.

@gerwaric
Copy link
Author

gerwaric commented Aug 8, 2023

I'm working on a change that might turn out to be simple right now. However, even that takes some iteration, and with the hard-coded refresh limit of 2 requests per minute, every time I change something it takes a few hours to test because I have hundreds of tabs.

@came-here-to-learn
Copy link

Can u make it read from some config file, or config menu?

So the numbers are not hardcoded, but rather selected by the user?

@came-here-to-learn
Copy link

came-here-to-learn commented Aug 8, 2023

Ok here are some technical comments:

  1. I login using POESSID
  2. I login to standard (does it matter?)
  3. I have over 500 tabs
  4. I have over 90 characters - this can be one of the reasons for the problems?
  5. I believe I get some error when the app tries to download character list? (I suspect it tries to hammer through all characters and I have too many?)

When I turn on the new version, it starts updating immediately; I tried it few times and managed to make a screenshot of two errors.

22222

I attach the screenshot above. Sorry that it is hard to read, I just cant "capture" it before the application starts to hammer GGG with hundreds of requests.

Also if you want to hardcore the numbers, the place is here
I submittet a patch long time ago, but (1) I dont know how to build the application #33
(2) dont know how to make the application allow the user to customize the numbers
(3) it would be nice to now download "X tabs per Y seconds". We also need "after J tabs wait for I seconds". Because GGG tests both if we dont open too many tabs per minute. They also dont allow us to have more than J tabs per hour.
(4) Also the app should not be so triggerhappy for users that have more than the default 8 (?) character slots.

Is there some way to build this app using some sort of an automated service? So without having to download 40 GB visual code and some unknown distributables?
I could submit my own patch, but dont know how to build.

@gerwaric
Copy link
Author

gerwaric commented Aug 8, 2023

Unfortunately I'm not a developer either. It took me almost a week just to figure out how to build the app locally. You can use QT Creator Community with the right packages selected, but you also need to setup OpenSSL.

It feels like this project needs significant updates that are beyond my ability. For one example, acquisition makes requests through www.pathofexile.com, but the latest developer documentation says third-party applications should use api.pathofexile.com. However, this would mean removing Steam and POESESSID as login options because those API endpoints require OAuth, but using OAuth also requires a special request to GGG for API access for the application. And so on. There are probably other issues with changes to items, currencies, etc.

I'll do what I can to fix the default startup behavior, but changes beyond that might be over my head. Like how the dynamic rate limiting worked for me, but caused problems for everyone else in this thread because I had only tested it on my computer with one account.

After causing people to hammer on GGG's servers, I'm reluctant to make changes without better code reviews and testing, because bugs like that put people's accounts and possibly the entire application's API access at risk.

@aiolos01
Copy link

aiolos01 commented Aug 9, 2023

The OATH issue in particular is currently unsolvable for standalone applications. It has nothing to do with coding skills. There are lots of people working on PoB and they can't make it work. GGG has promised changes but they aren't a priority (especially as GGG has stated multiple times how unhappy they are that tools like PoB even exist).

Don't worry about hammering GGG's servers. There aren't a lot of us using acquisition any more. And besides after a few seconds it becomes apparent that it's not working so we shut it down.

@gerwaric
Copy link
Author

I'm going to be offline for a couple week, but when I'm back in September I'll try to get some other simple changes to avoid the startup refresh.

@gerwaric
Copy link
Author

For anyone following this issue, I'm back to working on the problem. One major issue that was causing bugs was that my first implementation broke for people using Windows in other languages. I've got help from someone in the EU, so they have a different timezone as well as a different language setting. I'm hopeful that we will be able to get acquisition working again.

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.

6 participants