Memory Bloat can occur from cloning evmap.ReadHandle

antifuchs / ratelimit_meter

A leaky-bucket-as-a-meter rate-limiting implementation in Rust

MIT License

33 stars 6 forks source link

Memory Bloat can occur from cloning evmap.ReadHandle #39

Open lytefast opened 4 years ago

lytefast commented 4 years ago

Caused when you clone a ReadHandle in evmap: https://github.com/jonhoo/rust-evmap/issues/53 The fix is to bump to https://docs.rs/crate/evmap/10.0.2 : https://github.com/jonhoo/rust-evmap/pull/54

I started going down the road of patching this but it's a bit more involved:

Keys need to implement Hash (this was relatively simple as it's only really needed for the KeyableRateLimitState)
update ShallowCopy implementations with the breaking change to return a ManuallyDrop instance, as well as drop the mut reference.
- https://docs.rs/evmap/10.0.2/evmap/shallow_copy/trait.ShallowCopy.html

I'm not confident/familiar enough to though all implementations, but wanted to note it here while I try to upgrade to the governor implementation (which doesn't use evmap by default)

antifuchs commented 4 years ago

Thanks for the bug report! This is very interesting, and your pain reflects mine as I tried to upgrade evmap a while ago. This pain is part of the reason why I went with dashmap over in governor!

I think I'd be happy applying a patch if somebody were to submit one, but I think by now I'd strongly suggest going with governor... in there I had the chance to avoid some of the big mistakes I made in this crate. /:

lytefast commented 4 years ago

Yup. I'm running a build with governor, and so far there's no noticeable increase in work times. It takes a while to cause memory issues though, so no news on that front yet. Some context: At around 550 req/s it takes 2 weeks to run out of 40GB of available RAM.

antifuchs commented 4 years ago

Phew! That does sound pretty messy and subtle... until you run into it, which is when you really run into it.

I guess we might be able to construct a test to see how that behaves - I have some memory leak tests in governor and here; making a new one for cloning limiters seems worthwhile!

antifuchs commented 4 years ago

Side note: One of the reasons that I think governor is a far better design is that its RateLimiters can't be cloned - you have to Arc-wrap them and use them via references, which makes the whole story a bit easier to deal with (and also limits the available hashtable implementations... which might be a good thing, considering this issue).