Keeps all Limits in memory only #78
Conversation
alexsnaps
force-pushed
the
issue_74
branch
3 times, most recently
from
9e5c17f to
25bb776
Compare
There was a problem hiding this comment.
small doc fix should be done
limitador/limitador/src/lib.rs
Line 5 in 25bb776
|
Before merging this, I would tag the current HEAD of Alternatively, we could use the existing |
There was a problem hiding this comment.
Looking super good.
I failed to understand every bit, but I got the main idea. Limits live in the Storage struct (not a trait any more) and counters accessor is a trait (well, a smart pointer to an object implementing CounterStorage). Redis, InMemmory and Infinispam all implement CounterStorage
| } | ||
|
|
||
| pub struct AsyncStorage { | ||
| limits: RwLock<HashMap<Namespace, HashSet<Limit>>>, |
There was a problem hiding this comment.
Nice. HashSet enforces uniqueness for limits, doesn't it?
There was a problem hiding this comment.
@alexsnaps Looks like we including the limit's name into the hash for a Limit. I think two limits with everything same except name should be same as well right?
There was a problem hiding this comment.
@eguzki yes… but has it did before, as Limit was used as the key in a HashMap.
@rahulanand16nov I didn't change anything there really… but I tend to agree with you indeed. name doesn't semantically mean anything to the Limit, otoh the max_value is part of the PartialEq#eq impl. which I think is wrong. I'd rather not conflate these changes with this PR tho. But I think we should address these issues too!
| } | ||
| } | ||
|
|
||
| impl AsyncStorage { |
There was a problem hiding this comment.
Just for curiosity. Does it make sense to keep both async and sync storage version? If I am not wrong it is all about using the redis sync or async client. Shouldn't we choose one and stick to it?
There was a problem hiding this comment.
I don't think we can, no. Well... maybe we could, but we'd want to keep the async version for the server usage. But when all in-memory there is no added value (more on that in a second tho), as all operations are sync. So when using it as a lib, you should just be able to use the sync version and not require a reactor to run this in.
That being said, there is an async RwLock in tokio that we could use in lieu of the blocking one that we currently use. That'd make for different impl. of the two Storages. So, I guess I'm arguing to not only keep them, but have them drift further more.
There was a problem hiding this comment.
ok, got it... I think.
Thanks for the explanation
| limits.insert(limit.clone()); | ||
| self.counters.delete_counters(limits)?; | ||
|
|
||
| let mut limits = self.limits.write().unwrap(); |
There was a problem hiding this comment.
I am always a little hesitant with so many unwrap but I understand likelihood is low. Do you think we should manage results instead of panicking? Maybe I am being too conservative.
There was a problem hiding this comment.
Well… again, nothing new here. But these .unwrap() are on LockResult<RwLockReadGuard> which would return an Err if the lock is "poisoned". That happens when a thread panic'ed while holding an exclusive lock. This indicates that whatever this lock was guarding probably is "corrupted". But so that would mean: something went real bad in the past, for which we didn't cleaning recover from and some other thread is now trying to use that (possibly) corrupted state.
Until we find a strategy to recover from a poisoned state, I think panic'ing completely is probably the best option anyhow. One way would be to explicitly table flip and process::exit(1) on such a situation… unsure how much I want to litter the code with these conditions tho.
Again, if we want to change that (which I'm certainly open to, if only to start the discussion on how to achieve that), I wouldn't include these functional changes in this PR.
I'd rather use the the |
Part of addressing #74
I think the "easiest" way of reviewing this, is by starting with the changes to
StorageandAsyncStoragehere. The state of theLimits is now in theStorageitself… TheInMemoryStoreas somewhat of a copy of that state currently still, as I can't just reduce it to theTtlCacheusage…I tried to keep the diff small… for some definition of small. There are other smaller improvements that can be made here and there, but I'll keep those for some other change. I thought it'd be better to keep this "focused". So first step towards getting #74 done