Ratelimit-rewrite

[solemn cloak]

This is a breakout thread for @deep raft's http ratelimit rewrite started in #2418

(Some pings @keen trout, @topaz mango)

I am not sure if channels are the best way to handle ratelimits as things can happen in a uncontrolled order, but I think a rewrite to remove them would probably be quite large.

I have also been looking a bit at another "twilight-proxy"-like proxy called nirn-proxy which seems to be able to handle arbitary paths which is something which would be cool if we could add without compromising how well it works.

[topaz mango]

be careful with looking at nirn code btw, it is AGPL

I use nirn-proxy personally on top of serenity

[solemn cloak]

Yeah I am aware

Its rather annoying tbh.

[solemn cloak]

The issue with AGPL is that it is a bit strange what is part of the work covered by the license.

So we just need someone to read the code and describe it to us :)

[solemn cloak]

Hmm are any of the nirn-proxy people here.

I don't think so.

[deep raft]

You mean the mpsc channel? I don't see a problem, it's collected and split into individual queues asap (similar to the gateway queue).

[solemn cloak]

I have to read through your changes again but iirc you send your rate limit request through a queue, with a one shot to message back or am I completely off with that?

I have to read through your changes again but iirc you send your rate limit request through a queue, with a one shot to message back or am I completely off with that?

Hmm I did not send that twice??

And then we have some channels where we just run a sleep and then return which is more the place I could see issues or have those been removed?

[deep raft]

Queues are sequential, but do not block each other

You could have multiple requests in-flight in parallel within a queue, but that complicates staying in sync with the returned limits (i.e. they may return out of order)

Also most of the time requests are sequential, so it's a somewhat niche optimization

I did however remove the ratelimiters receive timeout. I figure that the http client can manages that (dropping the oneshot receiver to unstuck the ratelimiter)

[solemn cloak]

hmm if I am a consumer of a ratelimit and I want to know how long is left you can't do that with a channel even though it already is know right?

Or is there something that can change how long a ticket needs to sleep for after the initial creation?

And one I think we have seen is a 429 is hit and then the other requests in-flight for that endpoint still hit the 429 even though we know they will hit it, but I am not sure how to solve that.

I might just be talking shit though, because I am not sure how you make it in a better way without some rather hard central sync code.

[deep raft]

It is possible to implement something like that, but pending requests may unpredictably change your calculation. Note for example that my implementation does not decrement the queue's remaining/available count on making a request, but always assigns whatever Discord returns. Discord has stated to prefer a leaky bucket algorithm, but they have also stated that there may be exceptions to this.

[deep raft]

The actor (runner function) is now feature complete if anyone wants to take a look. The last piece was interactions bypassing the global limit. It's incredibly dense and will definitely need lots of integration tests ⚠️

[deep raft]

I added a new API in addition to acquire allowing for consumers to signal backpressure and inspect queues. Note that the predicate takes a function pointer and not a closure! You cannot reference any local variables with function pointers (the predicate runs in the actor, so closures would require allocations and Send bounds).

pub fn acquire(&self, path: Path) -> PermitFuture; // PermitFuture: Future<Permit>
pub fn acquire_if(&self, path: Path, predicate: fn(&Queue) -> bool) -> MaybePermitFuture; // MaybePermitFuture: Future<Option<Permit>>

I guess Queue will expose methods similar to what Bucket does today.

[deep raft]

Rethinking this, taking a closure is probably better cause the allocation should never be a performance problem. Oh well

[deep raft]

I have just marked the PR as ready for review and it is now also ready for early testing

[deep raft]

I chose to exclude this for now. The rate limiter needs to just be able to hash the path, so dynamic parsing can easily be slipt in later

[solemn cloak]

@deep raft Can I ask what the idea behind rehashing in actor.rs is?

[deep raft]

Discord's bucket hash is not globally unique, but only unique within the "top level resource".
However, HashMap<Path, Vec<u8>> and HashMap<(TopLevelResource, Vec<u8>), Queue> doesn't play well together because of ownership over the bucket hash (Vec<u8>). That's why I use the hashbrowns HashTable and hash by hand.

[solemn cloak]

Makes sense, I will continue looking through it

[deep raft]

I hope that I have not produced write only code

I honestly don't even remember how everything ties together. That is part of my motivation for writing integration tests haha

[solemn cloak]

I do have a in-progress comment about documenting the macros, mostly so it is clear when to use them and such as they can be a bit hard to decipher.

[deep raft]

I inlined Queue::pop into pop! (renamed from on_pop!). I this is a bit better

[solemn cloak]

Finally finished a review

Don't really have many comments, but i looks nice

[wanton seal]

@deep raft i deployed this fix to prod

because the gc removes queues bound to buckets it will eventually panic

atleast thats my thought

[deep raft]

Did the crash happen after a gc? It is meant to only drop unused queues…

[wanton seal]

not sure my traces weren't on

[deep raft]

I look at writing some tests for the garbage collection and see if I can make it crash

Ideally your fix should not be necessary, as it is more of a work around for the gc being overly aggressive

[deep raft]

GC occurs at a 6 hour interval so you could check if that lines up with your crash time

[wanton seal]

ill check in a moment

bot started ~3:45 and people started complaining around ~10 that it was broken

so it was likely a gc

i was asleep when it broke so i dont have an exact time

[wanton seal]

yeah this isn't true

at least with what i'm trying to do to replicate

[wanton seal]

got it to crash

[deep raft]

How did you replicate it?

[wanton seal]

im sometimes getting "hash is unchanged' too

while testing

[deep raft]

The GC left invalid bucket references to removed queues. Fixed by removing the reference from the bucket map too

[deep raft]

I think this stems from the invalid bucket references too, so it should be resolved

[deep raft]

@wanton seal are you still using the pr? And if so, did you run into any more issues?

[deep raft]

@keen trout I managed to shorten the in_flight branch. I believe that one is the most complicated so PTAL regarding documentation

[pure folio]

Is this ready to test yet?

[deep raft]

Yes! Some internals are pending final approval but the API/functionality should be mostly stable

[dawn vine]

been looking at this and i wonder for the http proxy if we still want this using that once the rewrite is in. it might be more stable (and correcter) for the proxy to not be tied to specific routes but do some generic parsing to find the major route and id. and collect them on the bucket id

[solemn cloak]

Yeah we have been talking about doing that for a while