From e63fb2407a1dd5de1bfe8d4d8b1976ce51022f0c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 10 Mar 2017 17:34:54 +0000
Subject: [PATCH] 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
---
 .../cache/strategy/local_cache_middleware.rb          |  8 ++++----
 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 @@ def call(env)
             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 @@ def test_local_cache_cleared_on_exception
             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
-- 
GitLab