Concurrency problem on exec_within_threshold method

[rodrigomonteirof]

I seted my threshold: 1 and interval: 60 and I got a concurrency problem on exec_within_threshold method when I start 5 sidekiq processes:

(INFO) time=2018-04-24 15:08:25 -0300 Msg=Checking contact for {:payload=>{:users=>["+5511958122379"]}}.
(INFO) time=2018-04-24 15:09:26 -0300 Msg=Checking contact for {:payload=>{:users=>["+5511941016555"]}}.
(INFO) time=2018-04-24 15:10:27 -0300 Msg=Checking contact for {:payload=>{:users=>
(INFO) time=2018-04-24 15:11:27 -0300 Msg=Checking contact for {:payload=>{:users=>["+5519983014013"]}}.
(INFO) time=2018-04-24 15:11:27 -0300 Msg=Checking contact for {:payload=>{:users=>["+5511941016555"]}}.
(INFO) time=2018-04-24 15:11:27 -0300 Msg=Checking contact for {:payload=>{:users=>["+5511958122379"]}}.
(INFO) time=2018-04-24 15:12:28 -0300 Msg=Checking contact for {:payload=>{:users=>["+5519983014013"]}}.
(INFO) time=2018-04-24 15:13:28 -0300 Msg=Checking contact for {:payload=>{:users=>["+5511941016555"]}}.

my code is:

ratelimit.exec_within_threshold('check_contacts',  threshold: 1, interval: 60) do                                                                 
      ratelimit.add('check_contacts')                                                                                                                 

      logger.info { "Checking contact for #{@body}." }                                                                                          

      ### My code here
end                 

And executed:

[1259, 1258, 1257, 1258, 1257].each { |p| MyWorker.perform_async(p) }

It seems that process read my redis at same time and execute ignoring rate limit configuration.

[jamespeerless]

We have seen a similar issue with this method. It's definitely not threadsafe. The issue is that multiple threads can read the current count before any of them get a chance to increment it using add. If the threshold is set to 20, current count is at 19, and you have three threads call exec_within_threshold at the same time then all three of the threads will see the count at 19 and all will execute the block when only 1 should have been allowed through.

The fix requires each thread to acquire a lock before being allowed to read the current count and to have them release it only after incrementing the count. I have implemented this fix in a fork of this project and opened a pull request here (https://github.com/ejfinneran/ratelimit/pull/33). You can use my fork or even just implement this directly in your project.

The only issue with my fix is that the counter will be incremented BEFORE yielding to your block. Normally I think this would be fine but I guess there could be cases where whatever action you are ratelimiting fails and you wouldn't have increased the count. This just means it's possible to perform slightly below the actual ratelimit which isn't too bad. You could also perhaps decrease the count by passing a negative number to add? I'm not sure if that's safe though.

Hope this helps.

[jamespeerless]

Have re-implemented my fix in a way that no longer requires a lock. We can use redis' Lua Scripting feature to implement an atomic count_and_increment_if_under_threshold function. The code and pull request is here https://github.com/ejfinneran/ratelimit/pull/35