Permalink
Browse files

introduce a body proxy to ensure that query cache is enabled during s…

…treaming
  • Loading branch information...
1 parent 4300855 commit 951e18abea9c116fc5d6b330ca1dcd2890abe006 @tenderlove tenderlove committed May 2, 2011
@@ -29,6 +29,14 @@ def cache
@query_cache_enabled = old
end
+ def enable_query_cache!
+ @query_cache_enabled = true
+ end
+
+ def disable_query_cache!
+ @query_cache_enabled = false
+ end
+
# Disable the query cache within the block.
def uncached
old, @query_cache_enabled = @query_cache_enabled, false
@@ -27,10 +27,31 @@ def initialize(app)
@app = app
end
- def call(env)
- ActiveRecord::Base.cache do
- @app.call(env)
+ class BodyProxy # :nodoc:
+ def initialize(original_cache_value, target)
+ @original_cache_value = original_cache_value
+ @target = target
+ end
+
+ def each(&block)
+ @target.each(&block)
+ end
+
+ def close
+ @target.close if @target.respond_to?(:close)
+ ensure
+ unless @original_cache_value
+ ActiveRecord::Base.connection.disable_query_cache!
+ end
end
end
+
+ def call(env)
+ old = ActiveRecord::Base.connection.query_cache_enabled
+ ActiveRecord::Base.connection.enable_query_cache!
+
+ status, headers, body = @app.call(env)
+ [status, headers, BodyProxy.new(old, body)]
+ end
end
end
@@ -10,6 +10,7 @@ class QueryCacheTest < ActiveRecord::TestCase
def setup
Task.connection.clear_query_cache
+ ActiveRecord::Base.connection.disable_query_cache!
end
def test_middleware_delegates
@@ -39,6 +40,31 @@ def test_cache_enabled_during_call
mw.call({})
end
+ def test_cache_on_during_body_write
+ streaming = Class.new do
+ def each
+ yield ActiveRecord::Base.connection.query_cache_enabled
+ end
+ end
+
+ mw = ActiveRecord::QueryCache.new lambda { |env|
+ [200, {}, streaming.new]
+ }
+ body = mw.call({}).last
+ body.each { |x| assert x, 'cache should be on' }
+ body.close
+ assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache disabled'
+ end
+
+ def test_cache_off_after_close
+ mw = ActiveRecord::QueryCache.new lambda { |env| }
+ body = mw.call({}).last
+
+ assert ActiveRecord::Base.connection.query_cache_enabled, 'cache enabled'
+ body.close
+ assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache disabled'
+ end
+
def test_find_queries
assert_queries(ActiveRecord::IdentityMap.enabled? ? 1 : 2) { Task.find(1); Task.find(1) }
end

21 comments on commit 951e18a

@radar
Contributor
radar commented on 951e18a May 4, 2011

This commit has broken tests for my application, visible at http://github.com/rails3book/edge-ticketee commit b129051cb7be7c1627b15e50b792518adabfc00e.

@tenderlove
Member

Can you provide any more information than this? What iss the error? How did it break it?

@tenderlove
Member

@radar I need more information to help you. Here is a comic that illustrates the problems I'm running in to

sadface

@foca
Contributor
foca commented on 951e18a May 4, 2011

I wanna have @tenderlove's babies when I grow up.

So I will steal them from him, while in a 🐻 disguise.

@radar
Contributor
radar commented on 951e18a May 4, 2011

Sorry for the short error report, it was very late in the day for me and I thought you may have known about an issue with it already (it seems obvious to me) and would be fixing it in a later commit.

The problem lies in the commit that I linked to which is in the chapter_7 branch for the repository, running bin/cucumber features/viewing_tickets.feature will duplicate this issue. If you're allergic to Cucumber then I will try my best to explain this issue.

I have a step called "Given I am signed in as them" which signs in as a user and then visits the homepage, which is ProjectsController#index. This action does Project.all against the database which returns an empty result set because, at this point, there are no projects in the database.

SELECT "projects".* FROM "projects" INNER JOIN "permissions" ON "permissions"."thing_id" = "projects"."id" WHERE "permissions"."thing_type" = 'Project' AND "permissions"."action" = 'view' AND "permissions"."user_id" = 1

Immediately after this step I create a new project using the "Given there is a project called "TextMate 2"" step. This uses Factory Girl to create the object, and I can see the query of the object going into the database:

SQL (0.3ms)  INSERT INTO "projects" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Wed, 04 May 2011 21:01:28 UTC +00:00], ["name", "TextMate 2"], ["updated_at", Wed, 04 May 2011 21:01:28 UTC +00:00]]

To the best of my knowledge, this statement indicates that there has been at LEAST an attempt to insert a record into the database. There are no rollback statements at all listed before or after this point.

Later on, I have another step called "Given I am on the homepage" which goes back to ProjectsController#index and rather than running the query against the database again, retrieves it from the cache:

CACHE (0.0ms)  SELECT "projects".* FROM "projects" INNER JOIN "permissions" ON "permissions"."thing_id" = "projects"."id" WHERE "permissions"."thing_type" = 'Project' AND "permissions"."action" = 'view' AND "permissions"."user_id" = 1

As far as I'm aware, there should be no queries that are cached (for this table?) when an INSERT is called. Therefore this query should not be retrieved from the cache but rather should be run against the database again.

So I did a git bisect on Rails to figure out the commit that broke it and it wound me up here.

Anything else?

@radar
Contributor
radar commented on 951e18a May 4, 2011

Please no "tl;dr" comments. You asked for it.

@radar
Contributor
radar commented on 951e18a May 4, 2011

Please no "tl;dr" comments. You asked for it.

@josevalim
Member

@radar, have you tried updating rack-test (to 0.6.0) and running tests again?

@radar
Contributor
radar commented on 951e18a May 4, 2011

@josevalim: rack-test was already at 0.6.0.

@josevalim
Member

Not in the Gemfile.lock in branch chapter_7. :)

@josevalim
Member

Ok, thanks! I will try to reproduce.

@josevalim
Member

Can you push an update Gemfile.lock to be sure we will be running on the same things?

@josevalim
Member

I have found the error. AP will push a fix soon.

@josevalim
Member

Thanks for the report @radar.

@radar
Contributor
radar commented on 951e18a May 4, 2011

Great! Thanks @josevalim and @tenderlove.

@tenderlove
Member

@radar thanks for the report! ❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️

Fixed on 38d92d7

@radar
Contributor
radar commented on 951e18a May 4, 2011

Thank you for the speedy fix! 🍰 You're the 💣 ❤️

@modsognir

@radar tl;dr

@tiegz
Contributor
tiegz commented on 951e18a May 3, 2012

Sorry to resurrect this conversation, but I just noticed this change after upgrading to 3.2. Running locally in dev or prod mode (w/passenger or thin), my query_cache doesn't get cleared between requests b/c close() isn't being called. Wondering if that expected behavior in 3.2?

@tenderlove
Member

@tiegz no, it isn't expected. Sounds like you have a middleware that isn't properly calling close. If you can, try to boil down an example and file a ticket please. :-)

@tiegz
Contributor
tiegz commented on 951e18a May 3, 2012

ah, thanks @tenderlove ! Tracked it down, and someone had added a simple middleware that wraps the body in a Rack::Response. It has a close method so not yet sure why that did it, but I could easily refactor this by not introducing the Rack::Response obj.

Please sign in to comment.