Added Prioritybased rate limiter

[hiteshk25]

No description provided.

[magibney]

Reviewed again with a strict focus on practicality.

I think this will work as intended. It would be a good idea to address at least the race condition I identified, and it'd be a trivial/minimal change to put the header-parsing logic in the PriorityBasedRateLimiter.handleRequest() method, updating that method to take HttpServletReqest param instead of String.

The rest of this seems ok for now, though I strongly suspect we'll want to fully revert this and re-approach with a mind to making RateLimiter API properly pluggable.

[magibney]

There's a race condition here that could leak CountDownLatch instances into the waitingList. (because we check priorityOneRequests count before inserting into waitingList, it's possible that by the time we insert, another thread would have decremented priorityOneRequests and polled the waitingList).

[hiteshk25]

We can sync release/acquire method but I was avoiding this.

1. As each release call will poll and then then notify the any waiting request.
2. Also release call will make it decrements the ActiveRequests call.

I'm assuming this is enough to cover any race condition, but let me know if i'm missing something here ?

[magibney]

There's definitely a race condition here.

      if (this.activeRequests.get() < this.totalAllowedRequests) {
        return nextInQueue();
      } else {
        // RACE CONDITION SOMWHERE IN HERE
        CountDownLatch wait = new CountDownLatch(1);
        this.waitingList.put(wait);
        return wait.await(this.waitTimeoutInMillis, TimeUnit.MILLISECONDS) && nextInQueue();

It is possible for a ton of BACKGROUND requests to come in find activeRequests full, then have all (or some) of the activeRequests complete, and poll waitingList before the BACKGROUND requests have been added to it.

I think it's an edge case, but it's definitely there, and could yield unpredictable behavior.

[hiteshk25]

Reviewed again with a strict focus on practicality.

I think this will work as intended. It would be a good idea to address at least the race condition I identified, and it'd be a trivial/minimal change to put the header-parsing logic in the PriorityBasedRateLimiter.handleRequest() method, updating that method to take HttpServletReqest param instead of String.

The rest of this seems ok for now, though I strongly suspect we'll want to fully revert this and re-approach with a mind to making RateLimiter API properly pluggable.

Thanks @magibney I have created a follow up ticket. Let's discuss and propose this in the solr-community.

[magibney]

The race condition is still not addressed. While considering whether it would be ok provisionally, I realized another related problem:

The rate at which CountDownLatch instances can be removed from the queue is limited by the actual throughput of work permits acquired. Under the (quite realistic) scenario of heavy BACKGROUND request load, you could easily end up adding more CountDownLatches to the queue than the executing requests would poll off the queue, allowing the queue to grow indefinitely.

The blocked background requests would fail properly in a reasonable amount of time, but the vestigial CountDownLatches would accumulate in the queue. The vestigial CountDownLatches would (FIFO) get notified despite the fact that their initiating request may have long since given up, allowing any actual waiting BACKGROUND requests (at the end of the queue) to languish, and could pile up requests in a way that the queue would become effectively a sink (and heap memory leak).

I'm pretty sure this is not just an edge case -- leaking memory is basically guaranteed if requests come in at a rate faster than the throughput at which they can actually execute (a common case for a rate limiter).

[hiteshk25]

The race condition is still not addressed. While considering whether it would be ok provisionally, I realized another related problem:

The rate at which CountDownLatch instances can be removed from the queue is limited by the actual throughput of work permits acquired. Under the (quite realistic) scenario of heavy BACKGROUND request load, you could easily end up adding more CountDownLatches to the queue than the executing requests would poll off the queue, allowing the queue to grow indefinitely.

The blocked background requests would fail properly in a reasonable amount of time, but the vestigial CountDownLatches would accumulate in the queue. The vestigial CountDownLatches would (FIFO) get notified despite the fact that their initiating request may have long since given up, allowing any actual waiting BACKGROUND requests (at the end of the queue) to languish, and could pile up requests in a way that the queue would become effectively a sink (and heap memory leak).

I'm pretty sure this is not just an edge case -- leaking memory is basically guaranteed if requests come in at a rate faster than the throughput at which they can actually execute (a common case for a rate limiter).

Thanks @magibney Nice catch. I have updated the probable fix

[magibney]

Ok, I'm comfortable with this. There's still a race condition that should probably be addressed, but I think it's basically benign.