Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why Thread.exclusive ? #19

Open
DanielVartanov opened this issue Aug 19, 2014 · 2 comments
Open

Why Thread.exclusive ? #19

DanielVartanov opened this issue Aug 19, 2014 · 2 comments

Comments

@DanielVartanov
Copy link

Hi,

I looked through the code and I wonder why do you use Thread.exclusive when dealing with the counter?

My first concern is that Redis itself has awesome atomic operations like INCR, SETNX and others. Calls to Redis does not require any critical section.

My second concern is that if Thread.exclusive was added for the sake of memory storage, then how do you share limit counter between processes?

Thanks,
Daniel.

@gevans
Copy link
Owner

gevans commented Aug 21, 2014

Hey Daniel,

Thanks for opening this issue. This is left over from the original implementation. Originally, memory storage was the only available backend. There's no shared counter between processes when using memory storage.

@lsimoneau, even though that was authored a little over a year ago, thoughts/recommendation on the Redis backend's thread-safety without the use of Thread.exclusive?

@lsimoneau
Copy link
Contributor

Hey, so it's a little vague since it's been so long, but looking back over my commits, I didn't pay any attention to the Thread.exclusive, I just abstracted out the business of the in-memory storage and then wrote the Redis one to conform to the same interface.

Looking at the Redis implementation, it appears I've taken some fairly careful precautions to avoid any potential concurrency issues (https://github.com/gevans/sidekiq-throttler/blob/master/lib/sidekiq/throttler/storage/redis.rb#L35-L53), so I doubt that the lock is required for the Redis backend to behave correctly in multithreaded environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants