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: 090ed153db/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
This commit is contained in:
parent
c8c1460f7a
commit
e63fb2407a
@ -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
|
||||
|
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user