-
Notifications
You must be signed in to change notification settings - Fork 335
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
Specs fail following upgrade from 4.3.1 to 4.4.0 #163
Comments
So in the meantime I switched over to using Redis as the cache store, but 4.4.0 still causes the specs to fail:
Why is this dependency introduced? |
Thanks for reporting this bug. I'll try to track it down. On Fri, Feb 12, 2016 at 12:30 PM Tirdad C. notifications@github.com wrote:
|
@tirdadc It's not obvious to me how the Can you please include a longer backtrace to help debug the issue? In particular, please include the lines including Thanks! |
@ktheory sure thing:
I'm using Redis for the cache store: |
I've got the exact same issue after upgrading from 4.3.0:
My class Rack::Attack
Rack::Attack.cache.store = ActiveSupport::Cache::RedisStore.new
Rack::Attack.cache.prefix = 'testapp:rack:attack'
# snip
end |
Ok, made some progress tracking this down. This'll affect anyone using ActiveSupport & not dalli. In defined?(::ActiveSupport::Cache::MemCacheStore) This causes I'm working on a patch. |
In v4.4.0, checking `defined?(ActiveSupport::Cache::MemCacheStore)` could trigger an error loading dalli, which isn’t needed. This fixes that bug, and prevents similar bugs by checking `store.class.to_s` rather than `defined?(klass) && store.is_a?(klass)`. Writing an automated test to ensure that dalli is truly optional is difficult, but I was able to recreate the dalli load error in v4.4.0 by running: gem uninstall dalli ruby -Ilib -ractive_support/all -ractive_support/cache/redis_store -rrack/attack -e 'p Rack::Attack::StoreProxy.build(Redis::Store.new)' Fixes #163
Please upgrade to v4.4.1. That should fix this. 😄 Thanks for reporting this and helping me track it down. |
I have these pretty standard specs:
and they worked fine prior to the upgrade. Now I get:
I'm not using memcached:
The text was updated successfully, but these errors were encountered: