RFR: 1731: Improve RestRequestCache rate limiter

[Magnus Ihse Bursie]

On Tue, 3 Jan 2023 18:15:36 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

> In the RestRequestCache, we are trying to limit non GET calls for a specific host and user to one call per second (and all calls, including GET, for a specific host and user, need to be serialized). This throttling is based on recommendations from GitHub (https://docs.github.com/en/rest/guides/best-practices-for-integrators?apiVersion=2022-11-28#dealing-with-abuse-rate-limits). The current implementation isn't really doing that however. Calls are serialized, but depending on timing, we can definitely send more than one non GET call per second, at least in bursts.
> 
> This patch tries to rework this, using two locks. During high contention, it's possible that this new implementation will be less favorable to non GET calls than the current implementation, but I'm pretty sure we will at least adhere to the recommendation of performing at most one non-GET call per second. See bug for breakdown of locking order.

Marked as reviewed by ihse (Reviewer).

Is this not slowing us down unnecessarily? If we have like an incoming stream of requests like this:
GET1 POST1 POST2 GET2 GET3 GET4
Then we will correctly make sure only a single request is sent at a time, and that the two post operations will have a delay between them. But that will also mean that GET2...GET4 will be artificially delayed, awaiting POST1 and POST2.

My understanding is that we should be able to do like:
GET1 POST1 (wait with POST2) GET2 GET3 POST4 (now enough time has passed) GET4

I think this would be easy to accomplish, as well. Just reverse the order on the post locks, so we first take the non-get lock to ensure one second has passed since the last POST operation, and then we take the general lock. That way additional GET requests can be handled while the POST operation is waiting for the full second to pass, but we will still in the end always need to hold the general lock, so we do not send a POST and a GET at the same time.

Actually, that is what you are doing. :-)

I got confused by:

 var authNonGetLock = authNonGetLocks.computeIfAbsent(authId, id -> new ReentrantLock());
 ```
thinking it was grabbing the lock.

-------------

PR: https://git.openjdk.org/skara/pull/1453