From f737dbb78c53bcce746cd6ed9493ff8c4c176da2 Mon Sep 17 00:00:00 2001 From: Genadi Samokovarov Date: Thu, 25 Sep 2014 18:13:18 +0200 Subject: [PATCH] Avoid rescue nil in the default throttled response It has a couple of cons: 1. If we slip a typo in the whole line, we won't easily catch it. Can you guys spot the problem problem in the following line? Chasing such issues is quite tricky. ```ruby retry_after = evn['rack.attack.match_data'][:period] rescue nil ``` 2. Throwing and catching an exception is quite slower than a new hash allocation, so there is a speed benefit too. We are guaranteed from Rack that env is a `Hash`, so we can even use `Hash#fetch`. ```ruby retry_after = env.fetch('rack.attack.match_data', {})[:period] ``` This reads better, but always allocates the default value hash, when the other version allocates it only when needed. If you prefer `Hash#fetch`, I'm fine with that, as long as we avoid `rescue nil`. --- lib/rack/attack.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index 4dab44d..c98f066 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -82,7 +82,7 @@ class Rack::Attack @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications) @blacklisted_response = lambda {|env| [403, {'Content-Type' => 'text/plain'}, ["Forbidden\n"]] } @throttled_response = lambda {|env| - retry_after = env['rack.attack.match_data'][:period] rescue nil + retry_after = (env['rack.attack.match_data'] || {})[:period] [429, {'Content-Type' => 'text/plain', 'Retry-After' => retry_after.to_s}, ["Retry later\n"]] }