Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InMemoryStore grows unbounded #71

Closed
alexsnaps opened this issue May 26, 2022 · 0 comments
Closed

InMemoryStore grows unbounded #71

alexsnaps opened this issue May 26, 2022 · 0 comments
Assignees

Comments

@alexsnaps
Copy link
Member

alexsnaps commented May 26, 2022

While we do have a TtlCache<Counter, i64>, the Set of Counter in limits_for_namespace: RwLock<HashMap<Namespace, HashMap<Limit, HashSet<Counter>>>> will grow unbounded. The TtlCache<Counter, i64>, limited to 1000 entries by default, will evict expired entries (when at capacity I guess?), the Set of Counters in our "main" datastructure is unbounded. Not necessarily a big issue, as it "only" will grow for "qualified" (probably not the best name) Limits (i.e. ones that have a counter per some arbitrary variable).

If we use only a single set of counters (i.e. the one in the main datastructure), we and once we have a non-blocking way of maintaining these, we probably should implement some eviction/expiry mechanism to reclaim memory of these counters.

I've debugged this using this test below… while the rate_limiter.get_counters("foo".to_owned()).unwrap().len() will return 0, going deeper in the InMemoryStore you'd see that the value of the <Limit, HashSet<Counter>> mapping for that Limit is a HashSet of size a 1000. It "merely" gets cleaned up in using the TTL of the cache in:

        for counter in self.counters_in_namespace(namespace) { // 1000 counters
            if let Some(counter_val) = self.counters.read().unwrap().get(&counter) { // which are all expired here
                let mut counter_with_val = counter.clone();
                counter_with_val.set_remaining(*counter_val);
                res.insert(counter_with_val);
            }
        }
#[cfg(test)]
mod tests {
    use std::collections::HashMap;
    use std::thread;
    use std::time::Duration;
    use crate::{Limit, RateLimiter};

    #[test]
    fn test_leak() {
        let rate_limiter = RateLimiter::default();
        let nothing: Vec<String> = vec![];
        let limit = Limit::new(
            "foo",
            2,
            1,
            nothing.clone(),
            vec!["id"],
        );
        rate_limiter.add_limit(&limit).unwrap();
        for o in 0..3 {
            for l in 0..3 {
                for i in 0..1000 { // grow the store even bigger, by making this >1000...
                    let mut map = HashMap::new();
                    map.insert("id".to_owned(), i.to_string());
                    assert_eq!(l == 2, rate_limiter // ... but requires removing this assertion, as limits won't be enforced, despite we holding a complete datastructure to be able to do so... but the TtlCache being used for expired counters, when at capacity, it considers these counters "expired". 
                        .check_rate_limited_and_update("foo", &map, 1)
                        .unwrap());
                }
            }
            if o == 2 {
                thread::sleep(Duration::from_secs(2));
            } else {
                thread::sleep(Duration::from_secs(1));
            }
        }
        println!("Counters: {}", rate_limiter.get_counters("foo".to_owned()).unwrap().len());
        println!("Counters: {}", rate_limiter.get_counters("foo".to_owned()).unwrap().len());
    }
}
@alexsnaps alexsnaps added kind/enhancement New feature or request participation/help needed Extra attention is needed labels May 27, 2022
@alexsnaps alexsnaps added this to the Limitador v0.4 milestone Jun 17, 2022
@alexsnaps alexsnaps added target/next and removed participation/help needed Extra attention is needed labels Aug 23, 2022
@alexsnaps alexsnaps moved this to In Refinement in Kuadrant Service Protection Aug 24, 2022
@alexsnaps alexsnaps moved this from Needs refinement to To do in Kuadrant Service Protection Oct 27, 2022
@alexsnaps alexsnaps mentioned this issue Jun 1, 2023
6 tasks
@alexsnaps alexsnaps moved this from To do to In Progress in Kuadrant Service Protection Jul 27, 2023
@alexsnaps alexsnaps self-assigned this Jul 27, 2023
@alexsnaps alexsnaps moved this from In Progress to To test in Kuadrant Service Protection Aug 8, 2023
@pehala pehala moved this from To test to Done in Kuadrant Service Protection Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

1 participant