Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix query cache to load before first request #33856

Conversation

eileencodes
Copy link
Member

In a test app we observed that the query cache was not enabled on the
first request. This was because the query cache hooks are installed on
load and active record is loaded in the middle of the first request.

If we remove the on_load from the railtie the query cache hooks will
be installed before the first request, allowing the cache to be enabled
on that first request.

This is ok because query cache doesn't load anything else, only itself
so we're not eager loading all of active record before the first
request, just the query cache hooks.

[Eileen M. Uchitelle & Matthew Draper]

In a test app we observed that the query cache was not enabled on the
first request. This was because the query cache hooks are installed on
load and active record is loaded in the middle of the first request.

If we remove the `on_load` from the railtie the query cache hooks will
be installed before the first request, allowing the cache to be enabled
on that first request.

This is ok because query cache doesn't load anything else, only itself
so we're not eager loading all of active record before the first
request, just the query cache hooks.

[Eileen M. Uchitelle & Matthew Draper]
@eileencodes eileencodes modified the milestones: 6.0.0, 5.2.2 Sep 12, 2018
@eileencodes eileencodes merged commit ce1248a into rails:master Sep 12, 2018
@eileencodes eileencodes deleted the fix-query-cache-to-load-before-first-request branch September 12, 2018 14:46
eileencodes added a commit that referenced this pull request Sep 12, 2018
…fore-first-request

Fix query cache to load before first request
@matthewd
Copy link
Member

I just checked the other places we install an executor/reloader hook inside an on_load, and those are okay:

ActiveSupport.on_load(:action_view) do
unless ActionView::Resolver.caching?
app.executor.to_run ActionView::Digestor::PerExecutionDigestCacheExpiry
end
end

app.reloader.before_class_unload do
ActionCable.server.restart
end

Those are both [only] arranging to clear out state that the relevant component may have accumulated, so it's fine to not invoke them if is hasn't been loaded yet.

This is the only case where we're setting state around the request, and therefore need it to always be present.

@rafaelfranca
Copy link
Member

Should not those hooks run on boot time in production? All models are loaded so the model that is loaded would execute the on_load hook.

rafaelfranca added a commit that referenced this pull request Sep 13, 2018
…ache

In production the query cache was already being loaded before the first
request even without #33856, so added a test to make sure of it.

This new test is passing even if #33856 is reverted.
@matthewd
Copy link
Member

@rafaelfranca yeah, this should only manifest when eager loading isn't on, so it's fairly low impact, but it's still a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants