Specifying default cache options no longer accidentally creates a DistributedStore (Fix for #166) #169

Closed
wants to merge 2 commits into
from

Projects

None yet

10 participants

@mjtko
mjtko commented Feb 20, 2013

This pull request includes the failing tests created in #166, along with a fix which makes the tests pass.

This patch causes the options to be extracted from the addresses arguments passed in to Store#initialize prior to calling the Redis::Factory::create method.

mjtko commented Feb 20, 2013

Doh. Tests don't pass. Will fix.

mjtko commented Feb 20, 2013

Removed the test that was added against redis-store. I believe that this fix should be confined to redis-activesupport.

I think you want to remove the test in redis-store/redis-activesupport/test/active_support/cache/redis_store_test.rb:46 which tests that the passed in default options actually do something.
The default options test is unrelated to the connection issue, my bad for grouping the original PR as one.

Your code fixes the test that you removed in 2a5cd56

mjtko commented Feb 20, 2013

Hmm, I'm not certain where or what we should be testing here to be honest. The change I've made fixes setting up an ActiveSupport cache with options (so doesn't touch on the redis-store tests) so I was preferring the tests you added in redis-activesupport -- but they don't seem to pass either.

I don't really want to dig into the implementation of the ActiveSupport cache and test @data directly.

@jodosha: do you have any suggestions as to what kind of test you think would suit this change?

On a different note, I'll stop being so lazy and do some local testing rather than relying on Travis. 😉

I'd love to see this get merged as we're running on a fork to use this pull request until it does.

mjtko added some commits Feb 20, 2013
@mjtko @mjtko mjtko Extract options before creating connection
Calls extract_options! on the addresses arguments before passing the addresses to the ::Redis::Factory.  This fixes #166 so default cache options can now be passed without causing a DistributedStore to be created with both the intended server as well as a spurious localhost connection. 
f637e56
@mjtko mjtko Added tests in order to support verifying that the ActiveSupport::Cac…
…he::RedisStore creates the correct kind of underlying store.
9c2b7a7
mjtko commented Feb 26, 2013

Ok, tests updated/created! Tests pass! Travis is happy!

@jodosha -- hopefully this is close to being accepted for merge now, anything you'd like me tweak?

Having the same problem here. Will this be merged soon?

@jodosha - this should be merged in ASAP.

Does this cause the configuration problem where accidentally it also tries to use redis on localhost?

e.g.

config.cache_store = :redis_store, "redis://redis-host:1234/0/cache", { expires_in: 90.minutes }
# in console
1.9.3-p194 :001 > Rails.cache
=> #<ActiveSupport::Cache::RedisStore:0x007fbfbc10c808 @data=#<Redis client v3.0.3 for redis://redis-host:1234/0, redis://127.0.0.1:6379/0>, @options={:expires_in=>5400 seconds}> 

I looked at the README for redis-rails. To me it looked like I could pass an redis url String as the 2nd argument, and a options hash as the 3th to config.cache_store. Which is wrong.

This is really tricky. It blew my app in production. It configured not only my production redis service on another host, but it also tried to reach localhost:6379, on which it got a connection error. I did not catch this in development, as I use redis on localhost.

Let me know if this is the same issue, otherwise I'll create a new issue.

mjtko commented Apr 10, 2013

@mlangenberg Yeah, I was encountering a similar problem, hence the patch. Without the patch, the RedisStore interprets the options hash as a further Redis server (allbeit one that uses all the defaults, ie. for localhost). I monkey patch this patch in for my apps - it's been working great so far. :)

Thanks! In case the patch cannot be merged quickly, can the repo owner please update the README to reflect this behavior?
If anyone follows the README and configures redis to use another host or port, it will blow up the application. You might not notice this until deploying to production.

Yeah it seems like @jodosha must be away from github for a while as I can't imagine this isn't being merged on purpose. Been running on CrowdFlower@e6437d8 for a while now... Glad @mjtko got this fix up even if it wastes some time to figure out you need it...

Having the same issue. Since lots of people use localhost for redis right up until production, finding this took some sleuthing. +1

mikegee commented May 30, 2013

Add me to the list of folks looking forward this getting merged.

@freegenie freegenie referenced this pull request Jun 14, 2013
Merged

Fixes #189 #190

Can we merge this? I've been running on my own branch in production which has this fix merged in for more than a month now.

Owner
radar commented Feb 13, 2015

Can this PR please be updated to be mergeable to master?

mjtko commented Feb 14, 2015

Doesn't look straightforward to update this given all the structural changes. Are we sure this is even still an issue in the latest version/layout of redis-store?

mjtko commented Feb 14, 2015

It's been a while since I've looked at this obviously -- this needs to be updated to be a PR against redis-activesupport now then, right?

mjtko commented Feb 14, 2015

Superceded by PR against redis-activesupport: redis-store/redis-activesupport#29. Closing this one.

@mjtko mjtko closed this Feb 14, 2015
@mjtko mjtko deleted the mjtko:connection_url branch Feb 14, 2015
mjtko commented Feb 14, 2015

(PS. thanks for getting on top of this @radar!)

Owner
radar commented Feb 14, 2015

No worries :) Thanks for the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment