diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2017-03-10 17:34:54 +0000 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-03-10 17:34:54 +0000 |
commit | e63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c (patch) | |
tree | 429ef67efcb58e772d73564d4a05d91ef9afd79f | |
parent | c8c1460f7a97c15dff641a30e63914991d407595 (diff) | |
download | rails-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.rb | 8 | ||||
-rw-r--r-- | activesupport/test/caching_test.rb | 11 |
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 |