Support ActiveSupport::RedisCacheStore #340
Conversation
Thanks!
Thank a lot for the feedback. I especially appreciate you noticing that, given that making the test suite robust was the main focus a couple of months ago (#293). Any other feedback about ways to improve are very welcomed!
Thanks for checking rubocop also. |
|
I want to code review and test this a bit more before merging. Will do as soon as I can. Will let you know. |
|
Sounds good. Let me know if you have questions, or if anything's unclear. |
|
I change my initializer to: throttle('req/ip', :limit => 2, :period => 10) do |req|
req.ip # unless req.path.start_with?('/assets')
end...and now I can reproduce the error reliably. If I have the app running, then stop and restart Redis, the second request produces the error. Subsequent requests reproduce it until the 10sec timeout period expires. Why? No idea. |
|
I put the repro on github here. |
|
Closing. See notes on #281. |
|
@brian-kephart I'm just debugging issues with Rails 5.2 using built in Redis cache handler and rack attack keys in Redis that don't expire. It looks like your PR could fix it. Why it was closed? |
|
The only errors I could reliably reproduce were solved another way, see #281 as mentioned above. I haven't seen these errors again despite weeks of use. I think @grzuy would be open to merging this, but only with a reliable repro and a test. Current tests pass using the RedisCacheStore, so if it's really not working then there's something not covered by the tests. |
|
@brian-kephart @morgoth I was able to add a test reproducing the issue, see commits in #350. This PR can no longer be merged because the original repo was deleted. I am cherry-picking the commit anyways. Thank you both! |
|
Thanks @grzuy for your patience and persistence on this! |
`ActiveSupport::Cache::RedisCacheStore` is not compatible with the version of Rack Attack we are using (v4.4.1) per rack/rack-attack#281. Users that had rate limits enabled might see `Redis::CommandError: ERR value is not an integer or out of range` because the `raw` parameter wasn't passed along properly. Let's partially revert the change to the original cache store until we can update to Rack Attack v5.2.3 that has support for `ActiveSupport::Cache::RedisCacheStore` via rack/rack-attack#340. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66449
Fixes #281. Thanks for the good test suite, it prevented many problems. This PR passes Travis & Rubocop.