Permalink
Browse files

clear and disable query cache when an exception is raised from called…

… middleware
  • Loading branch information...
1 parent e145acd commit b4ff82a79177757509cefa2b103ae56d84b84f6d @tenderlove tenderlove committed Aug 28, 2011
Showing with 33 additions and 1 deletion.
  1. +6 −0 activerecord/lib/active_record/query_cache.rb
  2. +27 −1 activerecord/test/cases/query_cache_test.rb
@@ -61,6 +61,12 @@ def call(env)
status, headers, body = @app.call(env)
[status, headers, BodyProxy.new(old, body)]
+ rescue Exception => e
@exviva

exviva Aug 28, 2011

Contributor

Wouldn't ensure be enough?

@dchelimsky

dchelimsky Aug 28, 2011

Contributor

Any reason not to do all of this as a matter of course in an ensure block? Then there's no need to do it in the close method, and this could just bubble exceptions out?

@tenderlove

tenderlove Aug 28, 2011

Owner

ensure happens before the method returns. We need to delay disabling the query cache until all data has streamed to the socket. Otherwise templates (erb, haml, etc) will not take advantage of the query cache. The only way that we can know when all data has been written to the socket is when the web server calls close on the body.

I wrote a blog post about how I don't like this api because of stuff like this.

+ ActiveRecord::Base.connection.clear_query_cache
+ unless old
+ ActiveRecord::Base.connection.disable_query_cache!
+ end
+ raise e
@brynary

brynary Aug 29, 2011

Contributor

Instead of "raise e", should this simply be "raise"?

My understanding of the difference is that "raise e" will rewrite the backtrace, whereas "raise" will not.

@tenderlove

tenderlove Aug 29, 2011

Owner

No, that's not the case.

zomg

end
end
end
@@ -13,6 +13,32 @@ def setup
ActiveRecord::Base.connection.disable_query_cache!
end
+ def test_exceptional_middleware_clears_and_disables_cache_on_error
+ assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache off'
+
+ mw = ActiveRecord::QueryCache.new lambda { |env|
+ Task.find 1
+ Task.find 1
+ assert_equal 1, ActiveRecord::Base.connection.query_cache.length
+ raise "lol borked"
+ }
+ assert_raises(RuntimeError) { mw.call({}) }
+
+ assert_equal 0, ActiveRecord::Base.connection.query_cache.length
+ assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache off'
+ end
+
+ def test_exceptional_middleware_leaves_enabled_cache_alone
+ ActiveRecord::Base.connection.enable_query_cache!
+
+ mw = ActiveRecord::QueryCache.new lambda { |env|
+ raise "lol borked"
+ }
+ assert_raises(RuntimeError) { mw.call({}) }
+
+ assert ActiveRecord::Base.connection.query_cache_enabled, 'cache off'
@thetamind

thetamind Aug 28, 2011

Contributor

The assert message here should be cache on.

@tenderlove

tenderlove Aug 28, 2011

Owner

oops, thanks!

+ end
+
def test_middleware_delegates
called = false
mw = ActiveRecord::QueryCache.new lambda { |env|
@@ -213,4 +239,4 @@ class QueryCacheBodyProxyTest < ActiveRecord::TestCase
assert_equal proxy.to_path, "/path"
end
-end
+end

0 comments on commit b4ff82a

Please sign in to comment.