Add new API for threadsafe tryCall.

[PrasadPereraCoveo]

This PR contains not a fix but more of a new API that provides same functionality of tryCall with thread safety.

The existing tryCall seems to be not thread safe and can run into race conditions and the limit counter may end up giving unpredictable results. This can be tested with the test method in TryUpdateAndVerifyLimitTest:: tryUpdateAndVerifyLimitMultiThread. The existing issue of the API is that we read the counter value from underling storage and verify if the counter has exceeded the limit and then update the counter later. This is not thread safe as many threads will do the same comparison locally in different times after reading the value.

The solution is to do a setAndGet one time operation of the counter(s) at the storage level. This provides the thread safety as now the storage is responsible of the atomicity of the operation. In Redis storage, this is done using https://redis.io/commands/eval that provides atomicity of multi-operations in elements.

One main difference compared to tryCall in this API is that the final counter value will rather be limit + cost than limit (in general use, limit + 1)

Pros of the new API:

• Thread safety in counters. From tests, it guarantees to enforce precise limits.

• Performance. Though we use Lua scripts, the performance seems to improve as the whole API will make one trip to the underlying storage to increment and check the counter value. The performance results are:

  INFO: tryUpdateAndVerifyLimit 1000000 times took 75665.8 ms (average of 0.0756658 ms per call)
  INFO: tryCall 1000000 times took 114792.2 ms (average of 0.1147922 ms per call)

  I have pretty much replicated the existing SpillwayTest to SpillwayTryUpdateAndVerifyLimitTest + a multi thread test.

[PrasadPereraCoveo]

@Sytten @putgeminmouth @

[Sytten]

I will do a review over the weekend

[dreisch-coveo]

@MCoulombe727 @dlafreniere Can you please assist in the review and/or recommend others?

[PrasadPereraCoveo]

@MiloszKul

[PrasadPereraCoveo]

Hey @Sytten, I agree that it has a lot of duplicated code and duplicated tests etc. We didn't want to break anything for existing API so that's why ended up adding a new one. I will point to the specific parts of the code that really points to where the fix is done :)

• updateAndVerifyExceededLimits vs. getExceededLimits in Spillway: This is where the current issue exists. In getExceededLimits, we first read the value, check if exceeds the limit, handle triggers and finally update the value (per thread). This is not thread safe. So the fix version of this updateAndVerifyExceededLimits sets the value by +cost (default 1) + check if it exceeds the trigger limit (and enforce it) and get the value in one operation on the storage. This is an atomic operation thus making it thread safe.

• addAndGetWithLimit vs. addAndGet in Capacity: Here, we increment the atomic value while enforcing the limit as apposed just increment. This is InMemoryStorage atomic operation for (set + enforce limit + get).

• addAndGetWithLimit vs. addAndGet in RedisStorage: Here, pipeline.eval executes the COUNTER_SCRIPT that provides the atomic operation for (set + enforce limit + get).

The rest is just the new API code and duplicate tests. We wanted to fix this as a part of our findings while integrating of the Spillway library in one of our services.

[PrasadPereraCoveo]

👋 @Sytten @putgeminmouth @MCoulombe727 @dlafreniere @dreisch-coveo I would like to get some views on this. Currently, I'm looking to add some simple sliding window concepts to this library. And it would be great if this get merged before that. Thx :)

[Sytten]

I really don't have the time to work on this until end of next week sorry.