Avoid re-running logic to determine Request#ip once it's been calculated the first time #2292
Conversation
jeremyevans
left a comment
There was a problem hiding this comment.
This is certainly an noninvasive way to add caching, but super is not all that optimized, and often ip is only called once per request. I'd prefer a conditional at the top of the current method, and setting the ivar in each explicit/implicit return. However, I don't object to this approach, so let's wait for feedback from @ioquatix and see what he prefers.
|
I am fine with this approach. The alternative is to move it into the The only downside to this change is that caching is injected around the computation with no way to change the computation itself. Personally, when I introduce some kind of cache, I'd prefer to have two methods, e.g. |
|
In general I like that approach (#ip/#compute_ip) too. However, this has the same issue you mention earlier -
As either the #ip/#compute_ip methods are both in Helper, and it becomes stateful; or, #compute_ip is in Helper and #ip is not, and we've changed the API to Helper as it no longer responds to |
|
In principle, in |
5c389b7 to
2e55bd9
Compare
|
Yep seems reasonable, sort of similar to my first draft but avoids I've force-pushed that. I made |
|
I'm okay with this, and even thought I suggested it, I also don't mind the simpler approach either. The benefit of this approach is that we removed the call to Let's see what @jeremyevans has to say about it. |
jeremyevans
left a comment
There was a problem hiding this comment.
Looking at other Request methods, the caching we are adding here is most similar to params, so the way @willbryant was originally adding caching was consistent with the current code. I didn't realize this at first, because of where the method was defined.
I worry that this changes behavior for applications that include a module in Request that overrides ip, because now their override is no longer used. So in attempting to make caching of ip automatic for subclasses, we are breaking one use case. That's not an automatic blocker, but we should consider whether that is an appropriate tradeoff. If so, we should at least note this as a backwards incompatibility in the CHANGELOG.
I don't have objections to the separate compute_ip method, though we don't use that approach for params.
Note that Helper is already stateful depending on your definition, it just uses env caching instead of ivar caching for handling state changes.
Considering all that, my preference would be for @willbryant's original commit, with the method moved near the def params that does the same thing. However, if we decide that automatic subclass caching is more important than backwards compatibility, I'm OK with this approach (still with the method moved).
7f8bf04 to
e9801b7
Compare
|
Done, and added |
…ted the first time
e9801b7 to
d63d3e8
Compare
|
Thanks for your contribution. |
As mentioned in #2288, this reduces the amount of redundant re-calculation needed when gems like rack-attack call
Request#ipmore than once.Feedback on the code factoring most welcome. This seemed like the least invasive way to do it; the usual simple
@ip ||= begin..endpattern inside the method wouldn't work due to thereturnstatements, but if we don't like the idea of the logic being in one place and the caching another, I can set the ivar in each return statement.I did feel like doing it like this makes it very easy to add caching to other methods included from Helper without rewriting them, though, which could be desired over time?