Don't force a connection to be established in the query cache middleware#25573
Don't force a connection to be established in the query cache middleware#25573sgrif wants to merge 2 commits into
Conversation
We shouldn't assume the environment in which the executor is being used, *especially* now that this is no longer a middleware. Active Record is meant to establish its connection lazily as needed, but the query cache middleware currently is forcing it, which can cause things like boot time to slow down as a database connection is needed to load routes, etc. The way query caching is handled is actually pretty leaky. If the result of `ActiveRecord::Base.connection` is different in the `run` and `complete` stages, we will have leaked enabling the query cache in the first connection, and potentially changed the state of the latter. This is equally leaky, but will enable the query cache on the new connection when it is established during the course of the block. I think the long term solution here is to have query caching decoupled from the connection adapter, and instead handle it at a higher level. That's a much larger refactoring though, and I need to explore whether its worthwhile and if there's the expectation of the query cache living on the connection adapter in the public API. In the short term, this solves the concrete problem at hand. Alternatives ------------ We could simply not enable the query cache if Active Record isn't already connected. I'm concerened that this would actually cause the query cache to be actually disabled in some applications though, and adds a very subtle implicit contract to the executor which I don't want to have.
|
I wanted to get some feedback on this before I merge it. |
Isn't this PR 90% of the way to doing that? Having the connection defer back to the registry for the truth of the enablement seems a small step; the only notable hurdles seem to be 1) a "global" But... don't we have to solve both of those to properly address the "what if 💯 to doing something about this, though... I felt very dirty leaving it in its current connection-hogging state -- just didn't want to distract the executor change. |
|
(agree the "only if connected" alternative is untenable: I'd guess 99% of applications are not connected when this runs... which only makes the forced-connection behaviour more egregious.) |
|
When I mean decouple it, I mean further than just having the connection point at the global registry -- I mean actually not have it be a thing on the connection at all, either w/ some sort of decorator or by handling it at the appropriate place in |
Yeah as long as the cache isn't actually shared, I'm not terribly concerned there. I can't think of a reasonable use case for only caching queries on one connection. |
|
This looks good to me. |
We shouldn't assume the environment in which the executor is being used,
especially now that this is no longer a middleware. Active Record is
meant to establish its connection lazily as needed, but the query cache
middleware currently is forcing it, which can cause things like boot
time to slow down as a database connection is needed to load routes,
etc.
The way query caching is handled is actually pretty leaky. If the result
of
ActiveRecord::Base.connectionis different in therunandcompletestages, we will have leaked enabling the query cache in thefirst connection, and potentially changed the state of the latter.
This is equally leaky, but will enable the query cache on the new
connection when it is established during the course of the block. I
think the long term solution here is to have query caching decoupled
from the connection adapter, and instead handle it at a higher level.
That's a much larger refactoring though, and I need to explore whether
its worthwhile and if there's the expectation of the query cache living
on the connection adapter in the public API.
In the short term, this solves the concrete problem at hand.
Alternatives
We could simply not enable the query cache if Active Record isn't
already connected. I'm concerened that this would actually cause the
query cache to be actually disabled in some applications though, and
adds a very subtle implicit contract to the executor which I don't want
to have.