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

feature: support for ActiveSupport::MemCacheStore #153

Merged
merged 9 commits into from
Feb 8, 2016
Merged

Conversation

elhu
Copy link

@elhu elhu commented Dec 8, 2015

Hello,

This PR adds support for ActiveSupport::Cache::MemCacheStore, which is Rails' wrapper around Dalli.
Because of Dalli lack of support for race_condition_ttl, we're stuck with MemCacheStore... hence this PR!

@elhu
Copy link
Author

elhu commented Dec 8, 2015

Hmm, there's a compatibility issue with Rails 3.x's ActiveSupport::Cache::MemCacheStore...
Working a fix, I hope to ship it tomorrow.

@ktheory
Copy link
Collaborator

ktheory commented Dec 8, 2015

Hmm, there's a compatibility issue with Rails 3.x's ActiveSupport::Cache::MemCacheStore...
Working a fix, I hope to ship it tomorrow.

Thanks for working on that.

@elhu
Copy link
Author

elhu commented Dec 8, 2015

I've investigated this a bit, and there's an inconsistency in Rails 3.x, with ActiveSupport::Cache::MemCacheStore.

When using the Rails 3.2 (ActiveSupport::Cache::MemCacheStore + MemCache backend), the following returns 0:

Rails.cache.delete("foo")
Rails.cache.increment("foo") # => 0

Whereas with every other cache stores, such as Dalli, it returns nil.

I'm not sure it's a good idea to deal with that specificity in Rack::Attack, but since Rails 3.2 is end-of-life, I don't think I can fix it there either.

I could create a new MemCacheProxy class to that effect, but it's only going to be useful for Rails 3.x projects with the MemCache backend... any opinion or alternative idea :) ?

@elhu
Copy link
Author

elhu commented Dec 16, 2015

I've addressed the issue by creating a dedicate MemCacheProxy class for the ActiveSupport::Cache::MemCacheStore + MemCache backend case.

The original reason for this PR is still solved, and now everything should work well even for Rails 3.2!

@jarthod
Copy link

jarthod commented Dec 18, 2015

👍

@ktheory
Copy link
Collaborator

ktheory commented Dec 21, 2015

@elhu Very cool. Didn't realize dalli didn't support race_condition_ttl! So I agree it's valuable to support MemCacheStore.

I'll review this as soon as I can (probably in the next couple weeks.)

If you can report success running this patch in a production application, that'd be helpful testing.

@elhu
Copy link
Author

elhu commented Dec 21, 2015

I'm on vacation this week, but I can start testing things out on a production app starting the week after that.

I'll adjust the PR if I find any issues, and report here if I don't find any.

@elhu
Copy link
Author

elhu commented Dec 29, 2015

I've brought my branch up to date with master, and fixed a couple more things.

I am also running the patch on a live web-app, and have no problems to report so far.

@elhu
Copy link
Author

elhu commented Jan 7, 2016

@ktheory did you get a chance to review this?
Once the PR is OK for you, let me know, I'll squash my commits together to avoid clutter in the master tree 😃.

@@ -3,6 +3,7 @@
source "https://rubygems.org"

gem "activesupport", "~> 3.2.0"
gem "memcache-client"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add memcache-client as a development dependency instead?

This file is auto-generated from Appraisals file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed that. Didn't know about appraisal, very interesting stuff!
I've also refactored the Rack::Attack::StoreProxy.build method, which was getting too cluttered in my opinion (but I could move that to a separate PR if you think it's necessary).

@elhu
Copy link
Author

elhu commented Jan 12, 2016

I had to revert my refactoring of Rack::Attack::StoreProxy.build, MRI 2.0.0 and JRuby do not behave like MRI 2.1.x and 2.2.x when calling Kernel.const_defined? with a nested constant name.

@elhu
Copy link
Author

elhu commented Feb 8, 2016

@ktheory Is there anything more you would like to discuss regarding this PR?

@ktheory
Copy link
Collaborator

ktheory commented Feb 8, 2016

@elhu Looks good! 🍰

I'll aim to publish a new release by the end of the week.

ktheory added a commit that referenced this pull request Feb 8, 2016
feature: support for ActiveSupport::MemCacheStore
@ktheory ktheory merged commit d65796b into rack:master Feb 8, 2016
@elhu
Copy link
Author

elhu commented Feb 9, 2016

Great, thanks!

@ktheory
Copy link
Collaborator

ktheory commented Feb 10, 2016

Just shipped v4.4.0: https://rubygems.org/gems/rack-attack/versions/4.4.0

@elhu
Copy link
Author

elhu commented Feb 11, 2016

Brilliant, thank you very much!

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

Successfully merging this pull request may close these issues.

3 participants