Sweep expired rate limiter buckets

[mkobetic]

This adds a janitor process to the rate limiter to prevent unbounded growth of the bucket map. My main concern was locking out request flow while iterating over the potentially large bucket map looking for expired entries.

We can definitely do the search under the read lock, and do the removal separately under a write lock. However that is still potentially blocking requests from new IPs. Moreover as soon as a new IP queues up a write lock request, any following read lock requests will be blocked as well (I'm not 100% sure about this but I don't see how else you can prevent write lock requests being preempted by read locks forever).

To address the write lock blocking I split the bucket maps into two. One will be subject to creating new IP entries, and the other will be subject to the expiration sweep. That way the writes and sweeps should not collide. The cost is having to read 2 maps to check for entry presence (and consequently 2 read locks) on every read access, but I think it's worth it to avoid unpredictable sweep pauses. The maps' roles are swapped after every sweep.

Notes

• We could try to be more clever about keeping the two maps roughly the same size, but maybe let's collect some metrics first to see whether it's worth it

Update

Also added another mutex at the Limiter level, to protect the swap of the two buffer maps. Otherwise a goroutine that is in the middle of fillAndReturnEntry could end up getting the same map in the second check. Consequently we have two layers of mutexes now so we need to be careful about locking them in a fixed order so that we don't deadlock.

[mkobetic]

Hm, even though the swap of the two Buffers maps is atomic, I think it may require a lock to synchronize with goroutines that are in the middle of fillAndReturnEntry.

[neekolas]

defer rl.mutex.RUnlock() would work here, since it is always getting unlocked at the end

[mkobetic]

It would, but I was avoiding defer on these short paths as we do elsewhere.