-
Notifications
You must be signed in to change notification settings - Fork 337
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
Ruby 2.5 compatibility? #253
Comments
Hi, Can you provide more detail on how are you configuring rack-attack's cache store? Thanks! |
Hi! Thanks for responding. I don't set anything for the cache store, so I guess it's using Rails.cache ? which is currently :file_store Here is my whole initializer # frozen_string_literal: true
module Rack
# rack-attack gem configuration
class Attack
throttle("search", limit: 1, period: 1.second) do |req|
"count" if req.path == "/api/v2/stores/search" && !Rails.env.test?
end
end
end |
I created a brand new test rails 4.2.10 app, using ruby 2.5.0, added rack-attack with the same config you provided and cannot reproduce the issue. Can you provide the specific puma config you have? |
Here's my config/puma.rb
It was on heroku. I just noticed that this does not happen in my local dev machine, so perhaps it is combination of something in my production specific env and ruby 2.5. Here are the ENV values:
Thanks again for looking up on this. |
Subscribed.. same issue here w/ Ruby 2.5. If I drop this section of code in the initializer, the apps works again:
Full initializer:
|
@waruboy Still no luck reproducing the issue here. Are you using resque and/or redis? Maybe you want to share |
@grzuy Sorry for just replying. I tried to update other related gems but still the problem persist. Here is my Btw if remove the
|
@waruboy Thanks for sharing. I put together brand new test rails app to attempt reproduce your issue but wasn't able. |
@waruboy Do you have a top level constant called I noticed in a previous version of ruby, I would get a ton of warnings, something like, IIRC, "top level constant Store referenced..." when rack attack was really looking for Redis::Store. Then it broke with this ruby. I don't need my |
@jeffblake ! I do have a model named "Store". Let me check |
yes, on my current production log I had similar warning like @jeffblake
unfortunately it's not possible for me to delete https://github.com/grzuy/rails_test_app_4_2_10/pull/1 |
@grzuy ah got it. the test app miss this part:
now if you use this branch you can reproduce the error |
it also reproduce the way it does not happen on dev, but happens on test and production Rails.env |
Nice job! Thanks both @waruboy and @jeffblake for helping reproduce the issue. |
@grzuy thanks for looking this up! |
Anyone using: * ruby 2.5 * redis * rack-attack without redis-store and using at least one throttle * having a toplevel class named Store will hit this ruby 2.5 bug https://bugs.ruby-lang.org/issues/14407 That's because of the following buggy behavior of 'defined?' under ruby 2.5: ``` $ ruby -v ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux] $ irb > class Redis > end => nil > class Store > end => nil > defined?(::Redis::Store) => "constant" > ::Redis::Store NameError (uninitialized constant Redis::Store Did you mean? Store) ```
'defined?' is buggy in ruby 2.5.0, which under certain circumstances users using rack-attack can hit. See issue rack#253. I reported (https://bugs.ruby-lang.org/issues/14407) and fixed (ruby/ruby#1800) the issue in ruby already, but i guess i would take some time before there's a new ruby release including that fix. So for now we would need to circumvent this bug by using 'const_defined?' instead of 'defined?' for this particular case. More details: Anyone using: * ruby 2.5.0 * redis * rack-attack without redis-store and using at least one throttle * having a toplevel class named Store will hit this ruby 2.5.0 bug https://bugs.ruby-lang.org/issues/14407 That's because of the following buggy behavior of 'defined?' under ruby 2.5: ``` $ ruby -v ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux] $ irb > class Redis > end => nil > class Store > end => nil > defined?(::Redis::Store) => "constant" > ::Redis::Store NameError (uninitialized constant Redis::Store Did you mean? Store) ```
Quick update: There is a PR that fixes this issue. I am waiting for a code review. |
Kinda amazing it goes up to ruby-lang bug |
Yup! Thanks again for helping reproduce the issue in the first place. |
[Fixes #253] Avoid 'defined?' buggy behavior in ruby 2.5.0
Fix for this has been released in |
@grzuy thank you very much~~ |
Hello. Thank you for this gem. I use it on my project and it is great!
Recently I tried to update the project to ruby 2.5. Unfortunately I immediately get the following error:
NameError · uninitialized constant Redis::Store
Could this be related to:
https://blog.bigbinary.com/2017/10/18/ruby-2.5-has-removed-top-level-constant-lookup.html ?
Complete stack:
The text was updated successfully, but these errors were encountered: