aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLin Jen-Shin <godfat@godfat.org>2017-03-10 17:34:54 +0000
committerLin Jen-Shin <godfat@godfat.org>2017-03-10 17:34:54 +0000
commite63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c (patch)
tree429ef67efcb58e772d73564d4a05d91ef9afd79f
parentc8c1460f7a97c15dff641a30e63914991d407595 (diff)
downloadrails-e63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c.tar.gz
rails-e63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c.tar.bz2
rails-e63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c.zip
Make sure local cache cleared even it's throwing:
We (GitLab) hit into an issue that somewhere in the middleware chain was throwing `:warden`, which was caught in the wrapping middleware, but `LocalCache::Middleware` was not aware of it. It should look like: ``` ruby result = catch(:warden) do @app.call(env) end ``` Source: https://github.com/hassox/warden/blob/090ed153dbd2f5bf4a1ca672b3018877e21223a4/lib/warden/manager.rb#L35-L37 Using `ensure` could make sure that we would always do the cleanup, and better yet, avoid `rescue Exception` which we all should know that could cause some issues which could be very hard to debug. Please check the discussion thread for more context: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1402#note_25128108
-rw-r--r--activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb8
-rw-r--r--activesupport/test/caching_test.rb11
2 files changed, 15 insertions, 4 deletions
diff --git a/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb b/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb
index 174cb72b1e..4c3679e4bf 100644
--- a/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb
+++ b/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb
@@ -28,13 +28,13 @@ module ActiveSupport
response[2] = ::Rack::BodyProxy.new(response[2]) do
LocalCacheRegistry.set_cache_for(local_cache_key, nil)
end
+ cleanup_on_body_close = true
response
rescue Rack::Utils::InvalidParameterError
- LocalCacheRegistry.set_cache_for(local_cache_key, nil)
[400, {}, []]
- rescue Exception
- LocalCacheRegistry.set_cache_for(local_cache_key, nil)
- raise
+ ensure
+ LocalCacheRegistry.set_cache_for(local_cache_key, nil) unless
+ cleanup_on_body_close
end
end
end
diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
index c543122d91..c67ffe69b8 100644
--- a/activesupport/test/caching_test.rb
+++ b/activesupport/test/caching_test.rb
@@ -47,6 +47,17 @@ module ActiveSupport
assert_raises(RuntimeError) { middleware.call({}) }
assert_nil LocalCacheRegistry.cache_for(key)
end
+
+ def test_local_cache_cleared_on_throw
+ key = "super awesome key"
+ assert_nil LocalCacheRegistry.cache_for key
+ middleware = Middleware.new("<3", key).new(->(env) {
+ assert LocalCacheRegistry.cache_for(key), "should have a cache"
+ throw :warden
+ })
+ assert_throws(:warden) { middleware.call({}) }
+ assert_nil LocalCacheRegistry.cache_for(key)
+ end
end
end
end