rack-throttle
rack-throttle copied to clipboard
dryruby
Concurrency issue in setting count (time_window.rb:14) / JRuby
I'm seeing what looks like a concurrency issue in a custom Limiter based on rack-throttle.
But I'm not 100% sure, so thought I'd ask issue here.
Specifically, the retrieval and storage of count values in time_window (for example) is not atomic, so it is possible for two simultaneous increments to occur but one of the increments will not be recorded.
This is what I'm seeing in my logs: I, [2014-08-14T16:49:04.491000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 11/15 => allowed? true I, [2014-08-14T16:49:05.201000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 12/15 => allowed? true I, [2014-08-14T16:49:05.799000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 13/15 => allowed? true I, [2014-08-14T16:49:05.806000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 13/15 => allowed? true I, [2014-08-14T16:49:07.001000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 14/15 => allowed? true I, [2014-08-14T16:49:08.630000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 15/15 => allowed? true I, [2014-08-14T16:49:10.463000 #5547] INFO -- : Rate limiting: 6061dcae08b043d9889169bd9f153fdc-d10de8798db7ced7b5850c6d7000f339:2014-08-14T16 count = 16/15 => allowed? false
The cache is a remote redis, being accessed via Ohm. Less than optimal, but I'm picking up someone else's code.
Also this is running in JRuby, and there are both hourly and daily limits in play.
The ruby threading model is not my forte, so I might be missing something obvious.
If by chance there two requests do come in at the same time (in a concurrent app) it is possible for them to both read the value of the current count as the same, and increment them equally. I.E what happened at request 13/15 for you. This probably isn't the best! Though, I believe in order to fix this you should have a "concurrent-resistant" cache.
I think there is some more concurrency safety nets that are needed for the concurrently safe cache to work, though rolling your own cache get/set might be a good solution.
@FreekingDean Just tried to solve the same problem in our application, but the only safe way to do this is to have both the get and set happen within the same critical section. In other words, even if the get and set operations are each thread-safe, reading and then writing back a value is not because the entire operation does not happen within a lock.
Something is needed at the top level -- in Rack Throttle -- to handle this.
Another option would be for the cache implementation to provide an increment method, which Rack Throttle can use to fetch, increment, and set the value in a single operation. That would allow the cache to provide a thread-safe, atomic way to handle this.
Right, unfortunately this would require some cache-specific code. Though it could be "stubbed" out for a more generic get/set and allowed to be over-ridden via cache specific code.