search

dryruby / rack-throttle

Rack middleware for rate-limiting incoming HTTP requests.
http://rubygems.org/gems/rack-throttle
The Unlicense
948 stars 115 forks source link

Concurrency issue in setting count (time_window.rb:14) / JRuby #10

Open eldritch-elbow opened 10 years ago

eldritch-elbow commented 10 years ago

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.

FreekingDean commented 8 years ago

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.

GuyPaddock commented 8 years ago

@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.

GuyPaddock commented 8 years ago

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.

FreekingDean commented 8 years ago

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.