Normalize counter values #199
Conversation
Codecov Report
@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 73.17% 73.36% +0.18%
==========================================
Files 30 30
Lines 5048 5065 +17
==========================================
+ Hits 3694 3716 +22
+ Misses 1354 1349 -5
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
limitador/src/storage/in_memory.rs
Outdated
| @@ -96,7 +96,7 @@ impl CounterStorage for InMemoryStorage { | |||
| delta: i64, | |||
| load_counters: bool, | |||
| ) -> Result<Authorization, StorageErr> { | |||
| let mut limits_by_namespace = self.limits_for_namespace.write().unwrap(); | |||
| let limits_by_namespace = self.limits_for_namespace.read().unwrap(); | |||
There was a problem hiding this comment.
This change introduces a race condition when we eventually update the counters here. By the time we get to our call .update on our AtomicExpiringValue references, their value could have already been updated by another thread. This is why we made .update(...) -> u64, i.e. return the value at that updating moment. But we now need to recheck we didn't pass the threshold... worst, if we did on say the second counter in our vec, what do we do with the increment to the first one?
So, tl;dr, I'm thinking lets leave grabbing the write lock for now and address the concurrency problem in a subsequent PR, wdyt?
* Only used in InMemory storage for normalizing counters * Other storage implementations return `Ok(())` * Included in `storage.add_limit`, unwrapping for now since it can't fail
* Not needed anymore since counters are normalized when limits are created
didierofrivia
force-pushed
the
normalize-counter-values
branch
from
9711f63 to
c96f478
Compare
This PR is part of the many changes in the data model, tracked by #182
There's no relevant change in performance, I've included a snapshot for reference:
