-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Description
Following on from #2287, the other thing we noticed is that all the logic in Rack::Request#ip is run for each call.
This is often not noticed as it may only be hit once, typically by a request logger. But it's not uncommon for there to be many calls.
For example, in rack-attack, every individual safelist_ip call adds a filter like this:
def safelist_ip(ip_address)
@anonymous_safelists << Safelist.new { |request| IPAddr.new(ip_address).include?(IPAddr.new(request.ip)) }
end
Note that each entry in the Safelist makes its own call to request.ip when invoked, which happens on every request. This significantly multiplies the call count and overhead. In our Rails app this added up to 7ms of overhead which was not counted in the request duration (since it was before request processing proper started) which was both surprising and hard to find.
rack-attack could be refactored to avoid this, but that's only going to help with the calls from rack-attack, not the combination of rack-attack and any other code using Request#ip.
So, I think the natural thing to do would be to cache the result of Request#ip. This seems like a fairly safe optimization - unless we have the expectation that some middleware will change something about the trusted proxy list (etc.) after calling Request#ip itself.
This seems to me kinda unlikely, and would be relying on internal implementation behavior, but I wanted to ask if we think that sort of thing might go on, before drafting a patch?