fixes #295: Use a semaphore to protect the Batcher from unbounded memory growth. #304
Conversation
| @@ -141,10 +143,11 @@ impl Batcher { | |||
| notifier: Default::default(), | |||
| interval: period, | |||
| priority_flush: AtomicBool::new(false), | |||
| limiter: Semaphore::new(1000), | |||
There was a problem hiding this comment.
we may want to make this configurable later. Is there a better default we can use for now?
There was a problem hiding this comment.
I did the same for the batch size, made it a 100... but it all really is a function of what the Limits these counters back look like. i.e. ones with many Conditions or large variable names, will just consume more memory. And these limits change during a Limitador instance "lifetime"... So other than delegating the issue to the user, I don't really see anything really. We could, as I did in for the in-memory one, do some broken maths on the memory available on the pod... but again, it's a huge guestimate :(
There was a problem hiding this comment.
Or we'd need to monitor memory usage and do it all conditional on that... Not sure I want to start going down that rabbit hole tho 🙈
There was a problem hiding this comment.
Maybe we could, for now, use the same max_cached_counters value. Gives the user some control, while avoiding leaking too many "implementation details" just quiet yet, wdyt?
There was a problem hiding this comment.
Sounds good. Updated.
chirino
force-pushed
the
fix-295
branch
2 times, most recently
from
3488c3a to
bb56676
Compare
| .remove_if(counter, |_, v| v.no_pending_writes()) | ||
| .is_some() | ||
| { | ||
| self.limiter.add_permits(1); |
There was a problem hiding this comment.
I think the example in the doc is somewhat oversimplified...
Reading the docs, .add_permits doesn't account for the initial permits provided (i.e. consider it a max), see here
So we need to do all the math ourselves, as this wouldn't account for re-adding permits back when counters expire for instance. I'd expect this would run out of permit after a while.
There was a problem hiding this comment.
Actually... thinking about it, it might be good enough. As we don't compact the queue anywhere 🤔
So that, if the entry is passed to the consumer, even if there are no pending writes for this entry e.g. because it expired, then this would also remove it from self.updates and increment the tickets back...
…nded memory growth. Signed-off-by: Hiram Chirino <[email protected]>
No description provided.